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

Disable parameterised tests selectively #170

Closed
wants to merge 8 commits into from
Closed

Disable parameterised tests selectively #170

wants to merge 8 commits into from

Conversation

Michael1993
Copy link
Member

@Michael1993 Michael1993 commented Feb 12, 2020

Mostly copy-paste work. Hope it's OK. Closes #163


I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

@nipafx
Copy link
Member

nipafx commented Feb 18, 2020

The initial code was really just to quickly show a non-declarative solutions. I pondered it for a while - here are my thoughts:

  • we surely need ways to express combinations beyond concrete tuples (e.g. "disable if number is in [1, 4] and text is "b")
  • I'm not sure whether we can do everything in just one class (I doubt it, actually)
  • maybe Disable should be generic, but then we need one type per number of test parameters (1-5 should be enough "for everybody")
  • are we rebuilding an assertion library? It feels like assertions are mostly the same thing, but I think they're not good at asserting combinations, which is essential to what we want to do

What about this?

@ParameterizedTest
@CsvSource({ "1, a", "1, b", "1, c", "2, a", "2, b", "2, c", "3, a", "3, b", "3, c" })
public void test(int number, String text) {
	Disable.whenPasses(() -> assertThat(number).is(5));

	Disable.whenAllPass(
		() -> assertThat(number).is(5),
		() -> assertThat(text).is("c"));

	Disable.whenAnyPasses(
		() -> assertThat(number).is(5),
		() -> assertThat(text).is("c"));
}

Compared to Disable.when(number, text).is(1, "a"):

  • more code for the user to write
  • less code for us to write
  • more powerful ~> fewer feature requests
  • presumably users already know their assertion library

Maybe I'm overlooking some obvious failure, but I really like it. I'm really curious to learn what you think!

@Michael1993
Copy link
Member Author

Michael1993 commented Feb 19, 2020

I mean I still don't fully understand the use of this feature. You could just write:

@ParameterizedTest
@CsvSource({"1, a", "2, b", "3, c", "4, d", "5, e", "6, f"}
public void test(int number, String text) {
  if (!(number > 1 && number <= 4 || text.equals("f"))) {
    //the actual test
  }
}

Isn't that effectively the same thing?

@nipafx
Copy link
Member

nipafx commented Mar 3, 2020

No, it's not the same thing. :) With an ordinary if the test is considered to have passed (i.e. "green"), whereas with Disabled, it gets marked as, well, disabled, which is closer to the truth.

Unless, as you mentioned elsewhere, we added a method skip() that throws the right exception. Then this may be pretty unnecessary. Let's see where #175 goes and pause this until then.

@Michael1993
Copy link
Member Author

This is not only outdated but the approach is outright wrong, so I'm closing this.

@Michael1993 Michael1993 closed this Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Parameterised tests selectively
3 participants