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

DisableIfArgument #368

Merged
merged 36 commits into from May 29, 2021
Merged

DisableIfArgument #368

merged 36 commits into from May 29, 2021

Conversation

Michael1993
Copy link
Member

@Michael1993 Michael1993 commented Oct 31, 2020

Closes #313


PR checklist

The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.

Documentation (general)

  • There is documentation (Javadoc and site documentation; added or updated)
  • There is implementation information to describe why a non-obvious source code / solution got implemented
  • Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
  • Only one sentence per line (especially in .adoc files)
  • Javadoc uses formal style, while sites documentation may use informal style (see Documentation: Personal or formal style #265)

Documentation (new extension)

  • The docs/docs-nav.yml navigation has an entry for the new extension
  • The package-info.java contains information about the new extension

Code

  • Code adheres to code style, naming conventions etc.
  • Successful tests cover all changes
  • There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks (see Require configuration failiure tests? #164)
  • Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Contributing

  • A prepared commit message exists
  • The list of contributions inside README.md mentions the new contribution (real name optional)

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

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
(no, not because this will make Sonar consider it 100% covered, why would you think that?)

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@Michael1993 Michael1993 marked this pull request as ready for review November 2, 2020 17:24
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Copy link
Member

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

Only minor things (no wonder after the stream ;) )

docs/disable-if-parameter.adoc Outdated Show resolved Hide resolved
docs/disable-if-parameter.adoc Outdated Show resolved Hide resolved
docs/disable-if-parameter.adoc Outdated Show resolved Hide resolved
docs/disable-if-parameter.adoc Outdated Show resolved Hide resolved
docs/disable-if-parameter.adoc Outdated Show resolved Hide resolved
docs/disable-if-parameter.adoc Outdated Show resolved Hide resolved
docs/disable-if-parameter.adoc Outdated Show resolved Hide resolved
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Nov 10, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@nipafx
Copy link
Member

nipafx commented Nov 10, 2020

Re documentation: There's a bit of textual and a lot of topical overlap between the two @DisableIf... extensions. Let's merge them into disable-parameterized-test-....adoc (with a similar title. But we need to keep disable-if-display-name.adoc around, so we don't break links. Let's just empty the file and point towards the new documentation.

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Copy link
Member

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

LGTM (except merge conflict ;) )

@Michael1993
Copy link
Member Author

LGTM (except merge conflict ;) )

Sorry, could you take a look at the issue (#313)? There is an ongoing discussion there about why this PR is incomplete - I'd appreciate your input.

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@nipafx
Copy link
Member

nipafx commented Dec 22, 2020

I hope to have answered all your questions. :)

Base automatically changed from master to main March 2, 2021 20:12
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
…cation-interceptor

# Conflicts:
#	README.md
#	docs/docs-nav.yml
#	src/main/java/org/junitpioneer/jupiter/params/DisableIfNameExtension.java
#	src/test/java/org/junitpioneer/jupiter/params/DisabledIfNameExtensionTests.java
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@Bukama
Copy link
Member

Bukama commented Mar 23, 2021

Just re-request my review if your done @Michael1993

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@Michael1993
Copy link
Member Author

Michael1993 commented Mar 23, 2021

Things still on TODO:

  • Add JavaDoc on annotations
  • Add new test file and gradle instructions to compile it with parameter naming information If anyone better versed in gradle wants to do this, please, help me.

Copy link
Member

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

I call this an approve as my comments are mostly personal point of view. I leave it up to you to update the docs.

Before merging we have to be aware of the placeholder

README.md Show resolved Hide resolved
docs/disable-parameterized-tests.adoc Outdated Show resolved Hide resolved
docs/disable-parameterized-tests.adoc Show resolved Hide resolved
docs/disable-parameterized-tests.adoc Outdated Show resolved Hide resolved
* The fourth invocation, because it has a parameter that matches ".*grew" - ends with grew.

Just like with `contains`, if any parameter value matches any expression from `matches`, the invocation gets disabled.

Copy link
Member

Choose a reason for hiding this comment

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

I would add some kind of theader here, because the not is not a note for the particular example above

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what you mean. Could you clarify?

Copy link
Member

Choose a reason for hiding this comment

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

For me I see some kind of break between line 244 (Just like..) and 246 (NOTE:) where the sentences are not connected by each other but from the formatting it seems like they belong together.

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@Bukama
Copy link
Member

Bukama commented Apr 8, 2021

Can you update the branch with current main to run with the changed java/ JUnit versions please.

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@Michael1993 Michael1993 added this to In progress in Exploring Io Apr 20, 2021
@nipafx
Copy link
Member

nipafx commented May 18, 2021

Looks really good, great work! Some small changes aside, I renamed it from @DisableIfParameter etc to @DisableIfArgument etc because the conditions target arguments (i.e. the values passed for the parameters) and not the parameters themselves.

@Michael1993 Michael1993 changed the title DisableIfParameter DisableIfArgument May 19, 2021
@nipafx
Copy link
Member

nipafx commented May 29, 2021

We still need a commit message. 😬

@nipafx nipafx merged commit b49ff0a into junit-pioneer:main May 29, 2021
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.

Explore use of InvocationInterceptor to disable (parameterized) tests
4 participants