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: clear target test runner state after each run #1749

Closed
wants to merge 2 commits into from

Conversation

pnadolny13
Copy link
Contributor

@pnadolny13 pnadolny13 commented Jun 5, 2023

The target test framework had weird behavior where all 14 default tests passed for me but when I looked closer only 1 table was getting created in the destination database. I figured out it was because the input file was getting stuck on the first test's input file.

The test overrides the runner input_filepath

runner.input_filepath = self.singer_filepath
to pass in the path to the test's JSONL data but it only gets used on the first test because the second test uses the runner while it still has self._input state from the previous run so it skips the logic for reading in the new file. Ultimately all following tests get no input so they silently pass but dont actually do anything.

I confirmed this fix worked in MeltanoLabs/target-snowflake#38.


📚 Documentation preview 📚: https://meltano-sdk--1749.org.readthedocs.build/en/1749/

@kgpayne
Copy link
Contributor

kgpayne commented Jun 6, 2023

@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.

@kgpayne
Copy link
Contributor

kgpayne commented Jun 6, 2023

Related to #1752

@pnadolny13
Copy link
Contributor Author

Closing in favor of #1752 and I also found another bug for tests that follow a pytest.raises( we weren't getting a reset, I fixed that in target-snowflake for now though.

@pnadolny13 pnadolny13 closed this Jun 6, 2023
pnadolny13 added a commit to MeltanoLabs/target-snowflake that referenced this pull request Jun 6, 2023
Closes #28

- implements a fix for the test suites not running properly, also added
it to the SDK meltano/sdk#1749
- implements a lot of the default test validates methods to make asserts
- comment out the tests that fail for legitimate bugs
- logged the bugs
  - #43
  - #41
  - #40
- I also logged
#42 because I
wrote a test to assert the exception but I'm not actually sure if we
want that behavior or not

---------

Co-authored-by: Ken Payne <[email protected]>
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