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

TST: Updating and fixing tests #234

Closed
wants to merge 1 commit into from
Closed

TST: Updating and fixing tests #234

wants to merge 1 commit into from

Conversation

thermokarst
Copy link
Contributor

Preemptively addressing fallout from qiime2/qiime2#494.

Pre- qiime2/qiime2#494, TestPlugin.transform_format & TestPlugin.get_transformer eschewed the SDK's transformer lookup machinery as a way to conveniently lookup transformers without exposing the less-than-illustrative function names (who wants to import a function named _15, anyway?). qiime2/qiime2#494 drops that ability, by making TestPlugin.get_transformer go "all-in" on the transformation API, validations and all. Possible paths forward:

  1. Close this PR, and IMP: TestPlugin.transform_format now validates output qiime2#494
  2. Accept defeat, merge this PR and IMP: TestPlugin.transform_format now validates output qiime2#494
  3. Rethink our transformer testing strategy (ugh). Maybe this involves keeping the older behavior, but renaming the test helpers to make the goal more clear.
  4. Add machinery to the framework to look up a transformer cleanly, but skipping all of the validation steps.
  5. Some other option I am not thinking of right now

Thoughts @ebolyen?

@thermokarst thermokarst added this to In Review in 2020.2 via automation Nov 12, 2019
@@ -198,7 +206,7 @@ def test_miseq_demux_dirfmt_to_slpspefdf(self):
def test_fastqmanifest_single(self):
_, dirfmt = self.transform_format(
CasavaOneEightSingleLanePerSampleDirFmt,
SingleLanePerSamplePairedEndFastqDirFmt,
SingleLanePerSampleSingleEndFastqDirFmt,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this appears to be the only "real" error in this test suite, so if we decide to close this PR let's add a TODO/issue to fix this little typo.

@ebolyen
Copy link
Member

ebolyen commented Nov 14, 2019

Yikes, let's try and figure this one out in person. I agree that importing _15 is less than helpful.

@ebolyen ebolyen added the stat:blocking-other This is blocking other issues/PRs/projects. label Nov 14, 2019
@ebolyen ebolyen moved this from In Review to In Analysis in 2020.2 Nov 14, 2019
@thermokarst thermokarst assigned thermokarst and unassigned ebolyen Nov 15, 2019
@thermokarst
Copy link
Contributor Author

Before closing this PR I want to open a replacement with the minor bugfix (identified inline, above). I will also open a PR on the framework with some comments to explain things a bit more (this comment here is a bit out of context, but will be remedied later!).

@thermokarst
Copy link
Contributor Author

Uh oh, I think I forgot what I was referring to above...

@thermokarst thermokarst moved this from In Analysis to In Development in 2020.2 Feb 14, 2020
@ebolyen ebolyen removed this from In Development in 2020.2 Mar 5, 2020
@ebolyen ebolyen added this to In Review in 2020.6 via automation Mar 5, 2020
@ebolyen ebolyen moved this from In Review to In Development in 2020.6 Mar 5, 2020
@thermokarst thermokarst mentioned this pull request Mar 18, 2020
@thermokarst
Copy link
Contributor Author

Superseded by qiime2/qiime2#527
Superseded by #242

@thermokarst thermokarst deleted the test-harness-updates branch March 18, 2020 22:30
@thermokarst thermokarst removed this from In Development in 2020.6 Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat:blocking-other This is blocking other issues/PRs/projects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants