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

Draft: Migrate test_cgroups.py and test_benchmark_definiton.py to pytest framework #908

Closed
wants to merge 7 commits into from

Conversation

Naman-Priyadarshi
Copy link
Member

I have successfully created a small prototype for test_cgroups.py which works for pytest. The test was running successfully on my Linux machine.

benchexec/test_cgroups_pytest.py Outdated Show resolved Hide resolved
benchexec/test_cgroups_pytest.py Outdated Show resolved Hide resolved
@PhilippWendler PhilippWendler changed the title Migrate test_cgroups.py to pytest framework Draft: Migrate test_cgroups.py to pytest framework Mar 28, 2023
@PhilippWendler PhilippWendler marked this pull request as draft March 28, 2023 13:28
@Naman-Priyadarshi
Copy link
Member Author

In this modified code, I have created a new fixture called enable_logging that enables logging for the specific test function that it is applied to. It sets the logging level to logging.DEBUG, which means that logging messages with severity DEBUG and above will be output.

The fixture also adds a finalizer function that disables logging after the test has completed, ensuring that logging is only enabled for the duration of the test.

To use this fixture, we simply add it as an argument to each test function that requires logging. When pytest runs the test, it will automatically call the fixture function and pass it as an argument to the test function. This will allow us to enable logging for specific tests without cluttering the output of other tests.

@PhilippWendler
Copy link
Member

In this modified code, I have created a new fixture called enable_logging that enables logging for the specific test function that it is applied to. It sets the logging level to logging.DEBUG, which means that logging messages with severity DEBUG and above will be output.

How does this look for the developer? How will they see the log?

To use this fixture, we simply add it as an argument to each test function that requires logging. When pytest runs the test, it will automatically call the fixture function and pass it as an argument to the test function. This will allow us to enable logging for specific tests without cluttering the output of other tests.

Is there a way to achieve this without having to add it to every single test manually?

The fixture also adds a finalizer function that disables logging after the test has completed, ensuring that logging is only enabled for the duration of the test.

Actually, this is not my main concern. If we have a decent handling of log output for tests, we can just use it for all tests and do not need to worry about fiddling with log levels at all. Only if we do not have a decent handling of log outputs for tests and need to disable logging, then we need to make sure it is actually disabled for each test.

@Naman-Priyadarshi
Copy link
Member Author

How does this look for the developer? How will they see the log?

There are multiple ways a developer can do this

  1. Use the -s option: When running pytest from the command line, they can use the -s option to enable stdout output capture. This will allow them to see any logging messages that were printed to the console during the test run.
  2. Use the caplog fixture: Pytest provides a fixture called caplog that allows to capture logging output from tests. We can use it to access the log records emitted by the logging module, and then perform assertions on them to ensure that the expected log messages were produced.

Is there a way to achieve this without having to add it to every single test manually?

Yes! We can achieve this task by defining a custom Pytest plugin. These plugins will allow us to extend Pytest's functionality by defining custom hooks that are called at various points during the test running process. We can define a custom plugin that sets up logging configuration for specific tests based on configuration options or markers, and then use the pytest_plugins list in a conftest.py file to load the plugin automatically for all tests in a directory or project.

@PhilippWendler
Copy link
Member

How would that caplog solution look like? Can you show a concrete example? Or tell me how to try it out?

@Naman-Priyadarshi
Copy link
Member Author

So, I tried adding the caplog solution to our test and used pytest-logger package.

We can use these command to read the caplog text file:
pytest test_cgroups_pytest.py --log-file=<path-to-log-file>

When we run our tests with this command, pytest will capture all log messages and save them to the specified file. We can then open the file in a text editor to read the log messages.

@PhilippWendler
Copy link
Member

So, I have now invested some time into playing around with this and reading the tutorial, and found the following:

  • By default, pytest does not show test output except for failing tests, and thus our reason for manually changing the log levels in tests is gone. This is good, and we can just get rid of that.
  • By default, pytest does show test output for failing tests, which is a nice and convenient improvement compared to the current state. It can still be disabled via command line or config file if this is ever necessary.
  • The log level for what is being captured and shown for failing tests can be easily set via pyproject.toml. This means we do not need to clutter all our tests with fixtures for logging.
  • --log-file works but it will just concatenate all logs from all tests with no distinction, which is rarely useful. Much nicer is -o log_cli=true or --log-cli-level=LEVEL.
  • Format of log messages can be improved for all these cases using config options.

So in summary, pytest is really nice in this regard, we can and want to get rid of all manual logging configs in tests, and we might want to set some nice logging format for pytest in pyproject.toml.

@Naman-Priyadarshi
Copy link
Member Author

Hi! sorry for being inactive, my current semester is ending soon so I'm busy with project submissions and other stuff! will work on this as soon as I get time.

@Naman-Priyadarshi
Copy link
Member Author

So, I have now invested some time into playing around with this and reading the tutorial, and found the following:

* By default, pytest does not show test output except for failing tests, and thus our reason for manually changing the log levels in tests is gone. This is good, and we can just get rid of that.

* By default, pytest _does_ show test output for failing tests, which is a nice and convenient improvement compared to the current state. It can still be disabled via command line or config file if this is ever necessary.

* The log level for what is being captured and shown for failing tests can be easily set via `pyproject.toml`. This means we do not need to clutter all our tests with fixtures for logging.

* `--log-file` works but it will just concatenate all logs from all tests with no distinction, which is rarely useful. Much nicer is `-o log_cli=true` or `--log-cli-level=LEVEL`.

* Format of log messages can be improved for all these cases using config options.

So in summary, pytest is really nice in this regard, we can and want to get rid of all manual logging configs in tests, and we might want to set some nice logging format for pytest in pyproject.toml.

Hi @PhilippWendler, I was wondering which format should we use and should we implement the caplog solution or not?

@PhilippWendler
Copy link
Member

I see no benefit in caplog so far. As for the format, it could simply be based on our standard log format.

@Naman-Priyadarshi Naman-Priyadarshi changed the title Draft: Migrate test_cgroups.py to pytest framework Draft: Migrate test_cgroups.py and test_benchmark_definiton.py to pytest framework Jun 1, 2023
@Naman-Priyadarshi
Copy link
Member Author

Made the required changes and migrated test_benchmark_definition.py too

@PhilippWendler
Copy link
Member

Looks fine!

@Naman-Priyadarshi
Copy link
Member Author

Hi @PhilippWendler! I was wondering if I should open a new PR for next workloads or add more commits to this PR? (P.S. Currently working on the next files to migrate)

@PhilippWendler
Copy link
Member

I think it makes sense to see this as a prototype, and create a fresh PR for the actual changes. We don't need the commits for trying out. And of course, we need the changes to the tests in the original files and not in additional new files.

@PhilippWendler
Copy link
Member

Will be superseded by other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants