search

jdamata / terraform-provider-sonarqube

Terraform provider for managing Sonarqube configuration
GNU General Public License v3.0
62 stars 51 forks source link

Fix project setting creation #202

Closed pnag90 closed 7 months ago

pnag90 commented 10 months ago
jdamata commented 9 months ago

@pnag90 I converted this to a draft PR. You should still get build results on commits. When its working and ready for review, set it back to ready for review :)

tbutler-qontigo commented 9 months ago

I have added a few comments but obviously it is up to @jdamata if he agrees with them or not. Other opinions are available :)

tbutler-qontigo commented 9 months ago

@pnag90 One other comment - since all the tests were passing before and you have not added any new tests then this suggests that the condition you are trying to fix (Which is unclear to me TBH) is not covered by the tests so....a) how does anyone know you fixed it at all? and b) It seems like it would make sense to add one or more tests to cover the scenarion to prove that it is fixed - and if they are run against the current code, they would fail.

Having green tests before your change and and the same tests still green afterwards suggests that either your change does nothing or that the existing tests are insufficient. I suspect the latter, in which case they are still insufficient!

pnag90 commented 9 months ago

@pnag90 One other comment - since all the tests were passing before and you have not added any new tests then this suggests that the condition you are trying to fix (Which is unclear to me TBH) is not covered by the tests so....a) how does anyone know you fixed it at all? and b) It seems like it would make sense to add one or more tests to cover the scenarion to prove that it is fixed - and if they are run against the current code, they would fail.

Having green tests before your change and and the same tests still green afterwards suggests that either your change does nothing or that the existing tests are insufficient. I suspect the latter, in which case they are still insufficient!

The tests were working for specific dummy examples of settings. When I tried to use the provider in a different project, start to fail with "real" use case settings.

jdamata commented 7 months ago

@pnag90 One other comment - since all the tests were passing before and you have not added any new tests then this suggests that the condition you are trying to fix (Which is unclear to me TBH) is not covered by the tests so....a) how does anyone know you fixed it at all? and b) It seems like it would make sense to add one or more tests to cover the scenarion to prove that it is fixed - and if they are run against the current code, they would fail. Having green tests before your change and and the same tests still green afterwards suggests that either your change does nothing or that the existing tests are insufficient. I suspect the latter, in which case they are still insufficient!

The tests were working for specific dummy examples of settings. When I tried to use the provider in a different project, start to fail with "real" use case settings.

Yes, I think what @tbutler-qontigo is rightly mentioning is that we would like to have those "real" use case settings that were failing for you to be added in. It would improve our test coverage posture and also confirm that your changes work for the issues you had. Of course sanitize any data you are using in your project

Also apology for the super late review. I reviewed this part way a long time ago and never came back to finish reading up. Thanks @tbutler-qontigo for reviewing this :)

pnag90 commented 7 months ago

Added tests for covering edition of a property setting