Skip to content

[test-stack] Implement inner class assertion and fix typo "doesNotContains"#18921

Merged
wing328 merged 10 commits intoOpenAPITools:masterfrom
Philzen:feature/implement-inner-class-assertion
Jun 16, 2024
Merged

[test-stack] Implement inner class assertion and fix typo "doesNotContains"#18921
wing328 merged 10 commits intoOpenAPITools:masterfrom
Philzen:feature/implement-inner-class-assertion

Conversation

@Philzen
Copy link
Contributor

@Philzen Philzen commented Jun 13, 2024

After some hours of wrestling Generics, this finally works as intended and mainly adds the following:

  • capability to run assertions on inner classes to JavaFileAssert
  • short-hand methods hasAnnotation(String) and doesNotHaveAnnotation(String) to MethodAssert and the new InnerClassAssert (these can be used to avoid writing .assertMethodAnnotations().containsWithName(String) and .assertMethodAnnotations().doesNotContainWithName(String), respectively.

Also did changes to harmonize and improve our JavaFileAssert API:

  • fixed the typo …doesNotContains… (that has been triggering my inner Monk for many time 😄 )
  • assertion classes that extend ListAssert are now pluralized (i.e. MethodAnnotationsAssert instead of MethodAnnotationAssert) to make it easier to understand whether you are asserting on a collection or an actual element
  • made method names consistent: assertSomething(…) now always returns a new assertion object (going one level deeper), while hasSomething(…) returns the same object (staying on the same level)

In general, there is more about this API that should be harmonized, but for starters i'm happy.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@Philzen Philzen force-pushed the feature/implement-inner-class-assertion branch from 7a251fa to 7e551c9 Compare June 13, 2024 22:26
@Philzen Philzen marked this pull request as ready for review June 13, 2024 22:40
@Philzen
Copy link
Contributor Author

Philzen commented Jun 13, 2024

@wing328 Kindly take a gander. Please merge before #18917 as it builds on the features that are introduced here.

Doing separate PRs as i want to start more clearly separating scope / concerns here, makes things easier in case anything comes up and a revert should be needed.

I was temped to press the merge button myself as i'm confident this PR is a non-critical feature addition to the test stack … however i don't want to take liberties here…

@wing328
Copy link
Member

wing328 commented Jun 15, 2024

can you please resolve the merge conflicts when you've time? after that i'll get it merged.

@Philzen Philzen force-pushed the feature/implement-inner-class-assertion branch from 965a512 to 964e070 Compare June 15, 2024 20:53
Philzen added 6 commits June 16, 2024 01:07
So .hasAssertion(String) can replace .assertMethodAnnotations().containsWithName(String)
This makes our the assertion API more consistent, in the way that
assertSomething("") will always return a different assertion type,
while hasSomething("") will always return the same type.
@Philzen Philzen force-pushed the feature/implement-inner-class-assertion branch from 410666a to 6af0eae Compare June 15, 2024 23:17
@Philzen
Copy link
Contributor Author

Philzen commented Jun 15, 2024

can you please resolve the merge conflicts when you've time? after that i'll get it merged.

Done!

Actually that merge conflict was a stroke of luck b/c i realized the API i introduced did not have the consistency i intended when i tried to apply the added hasAnnotation() short-hand method to the tests that were just merged in #18870 … now it's really beautiful, just look at 6af0eae

As part of that, i also harmonized the inconsistency between assert… and has…. Now assert… will always return a assertion object (going deeper), while has… gives back the same (staying on the same level). Writing tests is slowly becoming even more fun 😄

Updated the PR description and rebased #18917 on this, so that should go through easily.

@wing328 wing328 merged commit ec8998b into OpenAPITools:master Jun 16, 2024
@wing328 wing328 added this to the 7.7.0 milestone Jun 30, 2024
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.

2 participants