Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket 23 configure bluesky logging #114

Merged
merged 5 commits into from
Oct 28, 2024

Conversation

Chsudeepta
Copy link
Contributor

Description of work

Some system tests have been added for testing the bluesky logger

Ticket

Link to Ticket

Acceptance criteria

The system tests should run successfully as a complete set or when executed individually

System tests

Added tests to check the following:

  1. Log folder is created when logging is requested
  2. Log file is created when logging is requested
  3. The log message is present in the log file.

Code Review

  • Is the code of an acceptable quality?
  • Do the changes function as described and is it robust?
  • Have the changes been documented in the release notes?

Final Steps

  • Reviewer has moved the release notes entry for this ticket in the "Changes merged into master" section

Copy link
Contributor

@jackbdoughty jackbdoughty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After these changes, running this test module in one go I get test failures on lines 224 and 212 of assert os.path.exists(qualified_log_filename) == True AssertionError. I defined LOG_FILE_NAME = "bluesky.log"

test_bluesky.py Outdated
@@ -181,6 +187,45 @@ def _plan():
# Assert we successfully set npoints periods
self.assertEqual(g.get_number_periods(), npoints)

def test_GIVEN_logging_is_requested_THEN_the_log_folder_exists():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be def test_GIVEN_logging_is_requested_THEN_the_log_file_exists(self): -> None. Same for lines 201 and 213

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

this_function_name = sys._getframe().f_code.co_name
message = LOG_MESSAGE + this_function_name
logger.blueskylogger.info(message)
qualified_log_filename = os.path.join(log_path, LOG_FILE_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think LOG_FILE_NAME needs defining similar to the other environment variables at the top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added missed constant

@Chsudeepta
Copy link
Contributor Author

@jackbdoughty , I have corrected the issues, including the ruff ones

@jackbdoughty jackbdoughty merged commit 7e37dbc into master Oct 28, 2024
2 checks passed
@jackbdoughty jackbdoughty deleted the Ticket_23_Configure_Bluesky_Logging branch October 28, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants