fix: Better determine number of expected comments and webhooks in e2e test #3907
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what
Determine how many expected comments and webhooks there are in the events controller e2e test by making use of a variable that tracks whether or not plans need to be discarded.
why
This was a very subtle bug that I noticed when working on #2992. Right now we use the
ExpParseFailedCount
flag to decrement not only the number of webhooks we expect to have fired, but also the number of comments that are expected. When the controller fails to parse the command, it does not fire a webhook (https://github.com/runatlantis/atlantis/blob/main/server/controllers/events/events_controller.go#L577), however it does add a comment. We then unconditionally add1
to the expected number of comments to handle the comment for the cleanup of locks, but this does not happen in this case. Therefore two wrongs made a right, and the tests passed :)In the process of removing
--disable-apply
for #2992, I had the need to tweak these tests in such a way that we had a parse error, but still had locks to cleanup. In this case there was no right setting to make both number of webhooks and number of comments happy, thus necessitating another parameter,ExpNoLocksToDelete
that can vary independently fromExpParseFailedCount
.tests
All the changes are in unit tests, so unit tests passing should be sufficient.
The most straightforward test to demonstrate the old code has a bug is to add this simple test in
main
(which I ended up including in my PR)This fails with this message:
This is because we're unconditionally adding
1
with the expectation that every PR will need a comment to clean up, when it doesn't.The only Comment that
simple_with_allow_commands
creates is the response about the incorrect command. Adding1
to handle the cleanup then is not correct.references