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

Improve Cartesian test documentation #666

Merged
merged 8 commits into from Sep 19, 2022
Merged

Conversation

nipafx
Copy link
Member

@nipafx nipafx commented Sep 14, 2022

Proposed commit message:

Improve Cartesian test documentation (#665 / #666)

`@CartesianTest.MethodFactory`-annotated methods can be non-static
under certain circumstances, but the demo snippets showed non-static
factory methods that didn't match these and were thus wrong. This
change fixes that, which required moving the factory methods from
non-static inner classes into the outer class. For conistency and to
make things easier in the future, the same was done for all other
snippets as well.

The snippets were prepended with imports, which was incoherent when
the snippets turned from classes to just methods. Hence the imports
were removed as well, which has the eadded benefit of not having to
maintain the imports and making the snippets shorter.

Closes: #665
PR: #666

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
  • Site documentation in .adoc file references demo in src/demo/java instead of containing code blocks as text
  • Only one sentence per line (especially in .adoc files)
  • Javadoc uses formal style, while sites documentation may use informal style

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
  • 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)

@nipafx
Copy link
Member Author

nipafx commented Sep 14, 2022

Funny story: The failing demo demonstrated something, which I think we just don't do. 😳

The value attribute is required in the following example because the method parameter is declared as TemporalUnit, i.e. the interface implemented by ChronoUnit, which isn't an enum type.

@CartesianTest
void testExplicitEnum(@Enum(ChronoUnit.class) TemporalUnit unit) { ... }

But that fails because of the check whether the parameter type is actually an enum:

class CartesianEnumArgumentsProvider<E extends Enum<E>> implements CartesianParameterArgumentsProvider<E> {

	@Override
	public Stream<E> provideArguments(ExtensionContext context, Parameter parameter) {
		Class<?> parameterType = parameter.getType();
		if (!Enum.class.isAssignableFrom(parameterType))
			throw new PreconditionViolationException(
				String.format("Parameter of type %s must reference an Enum type", parameterType));
		// ...
	}

What's up with that?

@nipafx nipafx linked an issue Sep 14, 2022 that may be closed by this pull request
@Michael1993
Copy link
Member

Funny story: The failing demo demonstrated something, which I think we just don't do. 😳

The value attribute is required in the following example because the method parameter is declared as TemporalUnit, i.e. the interface implemented by ChronoUnit, which isn't an enum type.

@CartesianTest
void testExplicitEnum(@Enum(ChronoUnit.class) TemporalUnit unit) { ... }

But that fails because of the check whether the parameter type is actually an enum:

class CartesianEnumArgumentsProvider<E extends Enum<E>> implements CartesianParameterArgumentsProvider<E> {

	@Override
	public Stream<E> provideArguments(ExtensionContext context, Parameter parameter) {
		Class<?> parameterType = parameter.getType();
		if (!Enum.class.isAssignableFrom(parameterType))
			throw new PreconditionViolationException(
				String.format("Parameter of type %s must reference an Enum type", parameterType));
		// ...
	}

What's up with that?

Happens too early - that check is only supposed to happen if the annotation has no parameters.

@nipafx
Copy link
Member Author

nipafx commented Sep 15, 2022

Do you have time to fix this? For simplicity's sake, we can do it as part of this PR.

@nipafx
Copy link
Member Author

nipafx commented Sep 15, 2022

In Discord (❓) @Michael1993 pointed out:

image

The documentation on the website is missing this part:

(unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS) and the factory method is defined in the test class)

That's true for the documentation of the old Cartesian test extension. Is the addition correct there as well?

@Michael1993
Copy link
Member

In Discord (❓) @Michael1993 pointed out:

image

The documentation on the website is missing this part:
(unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS) and the factory method is defined in the test class)

That's true for the documentation of the old Cartesian test extension. Is the addition correct there as well?

No, in the old version it always had to be static, then someone suggested that it could be non-static when we were doing the new version.

@nipafx
Copy link
Member Author

nipafx commented Sep 16, 2022

Ok, then the documentation is correct as proposed in this PR. ✅

@nipafx nipafx added the full-build Triggers full build suite on PR label Sep 16, 2022
@Bukama Bukama merged commit 2038be8 into main Sep 19, 2022
@Bukama Bukama deleted the issue/665-cartesian-test-docs branch September 19, 2022 17:38
Bukama pushed a commit to Bukama/junit-pioneer that referenced this pull request Sep 20, 2022
…er#666)

`@CartesianTest.MethodFactory`-annotated methods can be non-static
under certain circumstances, but the demo snippets showed non-static
factory methods that didn't match these and were thus wrong. This
change fixes that, which required moving the factory methods from
non-static inner classes into the outer class. For conistency and to
make things easier in the future, the same was done for all other
snippets as well.

The snippets were prepended with imports, which was incoherent when
the snippets turned from classes to just methods. Hence the imports
were removed as well, which has the eadded benefit of not having to
maintain the imports and making the snippets shorter.

Closes: junit-pioneer#665
PR: junit-pioneer#666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-build Triggers full build suite on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated documentation with @CartesianTest
3 participants