8000 Fix running LS for tests in the new in_memory_localstack plugin by dfangl · Pull Request #9150 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Fix running LS for tests in the new in_memory_localstack plugin #9150

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 14, 2023
Merged

Fix running LS for tests in the new in_memory_localstack plugin #9150

merged 2 commits into from
Sep 14, 2023

Conversation

dfangl
Copy link
Member
@dfangl dfangl commented Sep 14, 2023

Motivation

With #9139, we introduced a new way to start LS in tests, via a pytest plugin.

However, the hook chosen to start LS, and to check the configuration option, might be executed before the configuration option is set.

The configuration options are set in the pytest_configure hook, with the following definition:

pytest_configure(config)
Allow plugins and conftest files to perform initial configuration.

This hook is called for every plugin and initial conftest file after command line options have been parsed.

After that, the hook is called for other conftest files as they are imported.

In community, this works, as the hook is specified in the initial conftest file, before the pytest_sessionstart hook is executed.

This, however, does not work if the test directory specified is a parent folder of the conftest file, as those will then only be imported during test collection, pytest_sessionstart takes place before test collection though.

Changes

  • Add log entry if the config option is not set, to be able to see the cause easier (in comparison to the plugin not being loaded, for example).
  • Start LocalStack with the pytest_runtestloop hook. This will take place after test collection, so after the conftest.py files in subdirectories are imported, and therefore the configuration option to start LocalStack should be present in these cases.

@dfangl dfangl added the semver: patch Non-breaking changes which can be included in patch releases label Sep 14, 2023
@dfangl dfangl added this to the 2.3 milestone Sep 14, 2023
@dfangl dfangl requested a review from thrau September 14, 2023 14:19
thrau
thrau approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member
@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM!

localstack/testing/pytest/in_memory_localstack.py Outdated
if not session.config.option.start_localstack:
LOG.info("Configuration option to start LocalStack not set.")
Copy link
Member

Choose a reason for hiding this comment

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

nit: should we maybe invert the info, i.e., only log IF it is set? otherwise you'll see this every time you run unit,cli, or bootstrap tests, and that may be a bit misleading.

Copy link
Member Author
@dfangl dfangl Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The reason this log was introduced, was that I had a hard time to debug why LS was not starting. In our case, because the config option was not set, but it could also have been that the plugin was not picked up, or some other reason.
If LocalStack would try to start, we should see that in the log anyway. However I see the issue with other tests.

What about a message like "LocalStack was not configured to start for this test suite"? Would that be less misleading?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed in slack, and agreed upon a message on module load, to log the plugin load.

@coveralls
Copy link
coveralls commented Sep 14, 2023 •
edited
Loading

Coverage Status

coverage: 80.126% (+0.003%) from 80.123% when pulling 20d041c on test-startup-fix into 0cdaf55 on master.

@github-actions
Copy link

LocalStack Community integration with Pro

       2 files         2 suites   1h 11m 1s ⏱️
2 209 tests 1 712 ✔️ 497 💤 0 ❌
2 210 runs  1 712 ✔️ 498 💤 0 ❌

Results for commit 20d041c.

@dfangl dfangl merged commit 31a612e into master Sep 14, 2023
@dfangl dfangl deleted the test-startup-fix branch September 14, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thrau thrau thrau approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
2.3
Development

Successfully merging this pull request may close these issues.
3 participants
@dfangl @coveralls @thrau
0