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

Have integration tests run with both scheduler and job queue #6787

Merged

Conversation

jonathan-eq
Copy link
Contributor

@jonathan-eq jonathan-eq commented Dec 12, 2023

Issue
Resolves #6756

Add pytest scheduler custom mark and fixture, indicating that an integration test will be ran with both old job queue and new scheduler.
The commit in this PR also moves all integration tests (atleast those marked with integration_test) to a new integration test directory.

tests/conftest.py Outdated Show resolved Hide resolved
@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch 9 times, most recently from 83f0daf to 40a8c94 Compare December 13, 2023 11:16
@jonathan-eq jonathan-eq changed the title Add pytest scheduler custom mark Have integration tests run with both scheduler and job queue Dec 13, 2023
@jonathan-eq jonathan-eq self-assigned this Dec 13, 2023
@jonathan-eq jonathan-eq marked this pull request as ready for review December 13, 2023 11:18
@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch 2 times, most recently from d965583 to 73d6c6d Compare December 13, 2023 11:28
@berland
Copy link
Contributor

berland commented Dec 13, 2023

Recommend to do pre-commit install in your local project folder, and it will be harder to miss on style issues :)

@berland
Copy link
Contributor

berland commented Dec 13, 2023

The commit in this PR also moves all integration tests (atleast those marked with integration_test) to a new integration test directory.

Should the moving around of tests be in a separate commit?

@jonathan-eq
Copy link
Contributor Author

I only moved parts of the files, but that is by far the main part of the commit. I think I would reword the commit message instead

@berland
Copy link
Contributor

berland commented Dec 13, 2023

For the tests that use the pytest snapshot feature, you need to git mv the snapshot files as well.

pyproject.toml Outdated Show resolved Hide resolved
@@ -245,6 +245,20 @@ def excepthook(cls, exc, tb):
monkeypatch.setattr(sys, "excepthook", excepthook)


@pytest.fixture(params=[False, True])
def try_queue_and_scheduler(request, monkeypatch):
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, the test that is being decorated with this fixture also needs to specify the monkeypatch fixture. Don't know why (otherwise it will silently ignore the request to enable scheduler).

@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch 4 times, most recently from b93ade0 to b0ef19e Compare December 14, 2023 08:34
@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch 3 times, most recently from ab0b359 to dff5199 Compare December 14, 2023 11:16
@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch from dff5199 to 1660d9c Compare December 14, 2023 12:04
@pytest.mark.integration_test
def test_that_surfaces_retain_their_order_when_loaded_and_saved_by_ert(
copy_case
copy_case, try_queue_and_scheduler
Copy link
Contributor

Choose a reason for hiding this comment

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

monkeypatch needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to commit the changes...

@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch from 1660d9c to 2f19c04 Compare December 14, 2023 12:41
@berland
Copy link
Contributor

berland commented Dec 14, 2023

Still two snapshots missing?

@xjules
Copy link
Contributor

xjules commented Dec 15, 2023

There are snapshots to be updated @jonathan-eq, but very good work nevertheless!

@jonathan-eq
Copy link
Contributor Author

Yes, it seems they updated ES-MDA so I will wait until they update the snapshots

@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch from c006aab to 3c8e24b Compare December 19, 2023 11:50
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1221607) 83.86% compared to head (a98c8be) 83.89%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6787      +/-   ##
==========================================
+ Coverage   83.86%   83.89%   +0.03%     
==========================================
  Files         365      365              
  Lines       21440    21440              
  Branches      948      948              
==========================================
+ Hits        17980    17988       +8     
+ Misses       3166     3158       -8     
  Partials      294      294              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch from 3c8e24b to 966a609 Compare December 19, 2023 12:29
Copy link
Contributor

@berland berland left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch 5 times, most recently from d047b62 to a98c8be Compare December 20, 2023 13:51
This commit moves all integration tests (the ones marked with pytest.mark.integration_tests atleast) to a new directory tests/integration_tests.
This commits adds the pytest.mark.scheduler mark and scheduler fixture to some of the integration tests, so that they will be ran with both the scheduler and job queue.
@jonathan-eq jonathan-eq force-pushed the move-integration-tests-for-new-queue branch from a98c8be to 83134a0 Compare December 20, 2023 14:08
@jonathan-eq jonathan-eq enabled auto-merge (squash) December 20, 2023 14:09
@jonathan-eq jonathan-eq added the release-notes:skip If there should be no mention of this in release notes label Dec 20, 2023
@jonathan-eq jonathan-eq merged commit a810eb0 into equinor:main Dec 20, 2023
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:skip If there should be no mention of this in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identify tests that can be run for both queues, eg. using feature toggling
4 participants