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

feat: only and skip support #55

Closed
wants to merge 5 commits into from
Closed

feat: only and skip support #55

wants to merge 5 commits into from

Conversation

ebebbington
Copy link
Member

@ebebbington ebebbington commented Aug 24, 2020

Fixes issue 5

WIP

Developments so far

  • Implemented only usage
    • Only a single suite or case can be only'ed, if more than 1 is, theen rhum throws an error
    • Will show all other tests as ignored
  • added skip support
    • same as above but as many test *'s can be skipped
  • Temporarily changed the output of rhum, see below

What's left

  • Complete and fix integration tests for only
  • add unit and integration tests for skip
  • add unit tests for only method
  • correct the output format, and adjust tests for this

Different types of output

image

image

mod.ts Outdated
Comment on lines 235 to 247
// FIXME(edward) Any test suite within a plan will a ctually hit this block, if there was a test suite before it. It should not hit this block
if (this.passed_in_test_plan && this.passed_in_test_suite) { // is a test case being skipped
this.plan.suites[this.passed_in_test_suite].cases!.push({
name,
new_name: this.formatTestCaseName(name),
testFn: cb,
only: true
});
} else if (this.passed_in_test_plan) { // is a test suite being skipped
this.passed_in_test_suite = name;
this.plan.suites![name] = {cases: [], only: true};
cb();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to figure this out

@ebebbington ebebbington changed the title Issue #5 only [WIP] Issue #5 only Aug 24, 2020
@ebebbington ebebbington added the WIP This item is a work in progress label Aug 24, 2020
@ebebbington
Copy link
Member Author

ebebbington commented Sep 23, 2020

Developments so far:

  • Implemented only usage
    • Only a single suite or case can be only'ed, if more than 1 is, theen rhum throws an error
    • Will show all. other tests as ignored
  • Temporarily changed the output of rhum which hasn't been best received, so it needs to be changed again - i'm proposing we ditch the current format but keep the test bit in, so it's easier to write tests for

What's left:

  • Complete and fix integration tests for only
  • add unit tests for only method
  • correct the output format, and adjust tests for this

@ebebbington
Copy link
Member Author

Decided to add skip support as well, because the only way only or skip will land, is if we ditch current output format, so may as well do it in one pr

@ebebbington ebebbington linked an issue Sep 23, 2020 that may be closed by this pull request
@ebebbington ebebbington changed the title [WIP] Issue #5 only [WIP] feat: only and skip support Sep 23, 2020
@crookse
Copy link
Member

crookse commented Oct 20, 2020

can we rework this PR to be pointed to v2? or should we trash it and start over with a new branch, but taking the logic from this branch as a starting point?

@ebebbington
Copy link
Member Author

Oh we can trash this PR, I was only keeping it open as a reference really until we landed your pr, if this PR is no use to you then feel free to close it and delete the branch 👍

@crookse
Copy link
Member

crookse commented Oct 20, 2020

i think it's still useful. we can keep it open as a reference for .only(). i like the edge cases so let's keep it open for now

@ebebbington
Copy link
Member Author

Alright

@crookse crookse changed the title [WIP] feat: only and skip support feat: only and skip support Oct 31, 2020
@crookse crookse added the Type: Minor Merging this pull request results in a minor version increment label Nov 1, 2020
@MVEMCJSUNPE
Copy link

Do we need only and skip properties for every test interface? It seems like it just overcomplicates things. For example, what happens if both only and skip are set to true?

@ebebbington
Copy link
Member Author

Hey, yes because it means any test suite or case can be skipped, and i believe i added checks for if both are used but i can't remember, this PR was from a while ago :P Tbh, we're only keeping this PR up for reference purposes for the rhum v2 PR, none of this code will end up in master and when rhum v2 is in master, this pr will be closed and the code lost forever 😢

@MVEMCJSUNPE
Copy link

I know. I just realized that this getting this to work is probably not possible if we keep using deno.std/testing as a base, unless we find some way to change the message format for deno.std/testing. The only way I can see to get this work is just to write our own base for testing instead of working off of deno.std/testing.

@MVEMCJSUNPE
Copy link

So basically we would have to write our own "deno:cli/rt/40_testing.js".

@crookse
Copy link
Member

crookse commented Nov 4, 2020

it might be that we don't need this feature now that i think about it. i say this because rhum v2 CLI has the --filter-test-case and --filter-test-suite options

@MVEMCJSUNPE
Copy link

I think those options achieve the same functionality as skip, but I'm not sure those options achieve the same functionality as only. I'm pretty sure ebebbington's code would have worked if Deno's testing api had more customizable format options.

@ebebbington
Copy link
Member Author

My pr was going to use a different test definition so we could use skip and only, but this meant the output would have changed in the shell for users

Where as now with Rhum v2, we have good output and can still ignore and only tests

@crookse crookse added the DO NOT MERGE DO NOT MERGE THIS PULL REQUEST label Nov 4, 2020
@ebebbington
Copy link
Member Author

For reference, this PR is only open to aid in eric's v2 PR, should any of the work i have done come in handy.

Once the v2 pr is merged, this will be closed without merging

@drashland drashland locked and limited conversation to collaborators Dec 27, 2020
@ebebbington ebebbington removed WIP This item is a work in progress Type: Minor Merging this pull request results in a minor version increment labels Dec 27, 2020
@drashland drashland unlocked this conversation Dec 27, 2020
@drashland drashland locked and limited conversation to collaborators Dec 27, 2020
@crookse
Copy link
Member

crookse commented Dec 2, 2021

not doing this anymore. v2 will be a test double framework

@crookse crookse closed this Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO NOT MERGE DO NOT MERGE THIS PULL REQUEST
Development

Successfully merging this pull request may close these issues.

3 participants