kubernetes-client icon indicating copy to clipboard operation
kubernetes-client copied to clipboard

Cleanup CRD generator tests

Open Donnerbart opened this issue 1 year ago • 6 comments

Description

Cleanup CRD generator tests to current code style, e.g.

  • use AssertJ
  • use final var for local variables
  • use List.of() and Map.of() for empty and singleton collections
  • in general fix warnings (mostly missing Optional::isPresent checks in this case)

Type of change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] Feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change
  • [x] Chore (non-breaking change which doesn't affect codebase; test, version modification, documentation, etc.)

Checklist

  • [x] Code contributed by me aligns with current project license: Apache 2.0
  • [x] I Added CHANGELOG entry regarding this change
  • [x] I have implemented unit tests to cover my changes (n/a, it's just test code)
  • [x] I have added/updated the javadocs and other documentation accordingly (n/a, it's just test code)
  • [x] No new bugs, code smells, etc. in SonarCloud report
  • [x] I tested my code in Kubernetes (n/a, it's just test code)
  • [x] I tested my code in OpenShift (n/a, it's just test code)

Donnerbart avatar Nov 25 '24 17:11 Donnerbart

There was a test failure in the Windows Build / Java 17 Maven LeaderElectorTest.jitterWithPositiveShouldReturnPositiveDouble:338 expected: <true> but was: <false> which looks unrelated to my changes. Is this a known flaky test?

EDIT: Another failure that seems to be unrelated:

2024-11-25T21:11:21.6203975Z [ERROR] Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 8.939 s <<< FAILURE! -- in io.fabric8.openshift.client.impl.OpenShiftConfigTest
2024-11-25T21:11:21.6208738Z [ERROR] io.fabric8.openshift.client.impl.OpenShiftConfigTest.shouldInstantiateClientUsingSerializeDeserialize -- Time elapsed: 8.464 s <<< FAILURE!
2024-11-25T21:11:21.6210624Z org.opentest4j.AssertionFailedError: expected: <null> but was: <default>
2024-11-25T21:11:21.6211908Z 	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
2024-11-25T21:11:21.6213448Z 	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
2024-11-25T21:11:21.6214825Z 	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
2024-11-25T21:11:21.6215953Z 	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
2024-11-25T21:11:21.6217128Z 	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
2024-11-25T21:11:21.6218286Z 	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
2024-11-25T21:11:21.6220001Z 	at io.fabric8.openshift.client.impl.OpenShiftConfigTest.shouldInstantiateClientUsingSerializeDeserialize(OpenShiftConfigTest.java:90)
2024-11-25T21:11:21.6221855Z 	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
2024-11-25T21:11:21.6222803Z 	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
2024-11-25T21:11:21.6223725Z 	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

https://github.com/fabric8io/kubernetes-client/actions/runs/12018627620/job/33503520990?pr=6663

EDIT: Another failure that looks unrelated JdkHttpClientAsyncBodyTest>AbstractAsyncBodyTest.consumeBytesProcessesLargeBodies:115 » Execution java.net.ProtocolException: protocol error: zero streamId for DataFrame https://github.com/fabric8io/kubernetes-client/actions/runs/12020938087/job/33510477323?pr=6663

Donnerbart avatar Nov 25 '24 18:11 Donnerbart

Is this a known flaky test?

Yes. There are a couple of flaky tests. Some have become more flaky now that everything is based on Vert.x.

manusa avatar Nov 26 '24 11:11 manusa

I've removed the changelog entry (just saw a similar one in a previous release).

@manusa Could you give me one concrete example how you would like the asserts to look like? I'm struggeling a bit with the deep nested structures and the many Optional<?> fields that have to be asserted.

How do you want to assert if you have to dive into multiple "branches" of the same object like in testClassNamesDisambiguationWithPackageNesting()? I'm not aware that e.g. extracing() can go back in the hierarchy for another assert. And with all the optionals I have no idea how something like hasFieldOrPropertyWithValue() could be used. Some variant of satifies() is the only way I know how to deal with these complex assertions.

Donnerbart avatar Nov 28 '24 18:11 Donnerbart

@manusa Could you give me one concrete example how you would like the asserts to look like? I'm struggeling a bit with the deep nested structures and the many Optional<?> fields that have to be asserted.

Hi @Donnerbart, I created #6689 which should provide you with some hints on how assertJ can be used (including the extraction and assertion) of optional values.

~~I will also check testClassNamesDisambiguationWithPackageNesting.~~

In general, if it's tricky to create an assertion with a single AssertJ statement it's a clear symptom that the test is checking too many things and should probably be split into multiple cases.

testClassNamesDisambiguationWithPackageNesting is a good example. In #6689 I split the test into multiple assertions since the original test was actually testing multiple things.

manusa avatar Dec 02 '24 13:12 manusa

Also, considering the complexity of the Test suite, it might be better to create multiple PRs instead of a single one with all the changes at once.

manusa avatar Dec 02 '24 16:12 manusa