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

fix: change runner scope to function for target tests #1752

Merged
merged 13 commits into from
Jun 7, 2023

Conversation

kgpayne
Copy link
Contributor

@kgpayne kgpayne commented Jun 6, 2023

@kgpayne kgpayne self-assigned this Jun 6, 2023
@kgpayne kgpayne closed this Jun 6, 2023
@kgpayne kgpayne force-pushed the kgpayne/fix-target-test-runner-reuse branch from 4921252 to 4eaca03 Compare June 6, 2023 09:33
@kgpayne kgpayne reopened this Jun 6, 2023
@kgpayne
Copy link
Contributor Author

kgpayne commented Jun 6, 2023

This is now failing on legitimate target tests 😅🎉 Will work on fixing those this afternoon 💪

@kgpayne
Copy link
Contributor Author

kgpayne commented Jun 6, 2023

Related to #1749 and this conversation:

@pnadolny13 @edgarrmondragon I think the issue and fix is a bit broader than resetting the target_input (though that obviously works).

A reset is needed because we made the runner instance a pytest fixture with scope="class". This is desirable for Taps as it means they can run once (to hydrate the runners record/state/messages attributes) and then run multiple tests on the same record set. It was done to minimise the number of times the Taps actually has to run. However, with Target tests and especially with the file-based ones, we actually want to allow the Target to sync multiple times with the different file inputs. So I believe the change needed is to set the runner scope to function when constructing Target classes 🙂 This has a similar effect to resetting the runners input as each runner is 'fresh' and so doesn't have any previous state.

Will raise a PR in target-snowflake to test the fix.

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #1752 (40b4cfa) into main (cf57439) will increase coverage by 0.04%.
The diff coverage is 93.39%.

❗ Current head 40b4cfa differs from pull request most recent head caf672c. Consider uploading reports for the commit caf672c to get more accurate results

@@            Coverage Diff             @@
##             main    #1752      +/-   ##
==========================================
+ Coverage   85.52%   85.57%   +0.04%     
==========================================
  Files          59       59              
  Lines        4892     4936      +44     
  Branches      803      809       +6     
==========================================
+ Hits         4184     4224      +40     
+ Misses        509      508       -1     
- Partials      199      204       +5     
Impacted Files Coverage Δ
singer_sdk/testing/factory.py 93.57% <92.78%> (-0.79%) ⬇️
singer_sdk/testing/__init__.py 100.00% <100.00%> (ø)
singer_sdk/testing/runners.py 86.91% <100.00%> (-2.81%) ⬇️
singer_sdk/testing/target_tests.py 93.87% <100.00%> (+1.02%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kgpayne kgpayne merged commit 6296599 into main Jun 7, 2023
@kgpayne kgpayne deleted the kgpayne/fix-target-test-runner-reuse branch June 7, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants