Integration tests proposal #5475
Conversation
There was a problem hiding this comment.
Thank you for working on this. I added a few comments. It would also be nice to have documentation on when we should use unit tests vs when we should use integration tests.
In the future, I also wonder if we can use k6 for any of the tests. We don't have to do it for this PR though.
There was a problem hiding this comment.
Few comments. I would also like to see a README.md where we could describe what tests are appropriate for this framework and what are the alternatives.
In general, I would be inclined to merge this and see how reliable it is in practice. We may tweak or revert it later, but either way it's a worthwhile experiment and good work driving and implementing this @wildum ! 🎉 👏
There was a problem hiding this comment.
Thanks, looks good! I'm happy for it to be merged once @thampiotr also approves.
Btw I wonder if we can also test kubernetes components one day too, using something like k3d. Testing kubernetes-related things locally has been a bit of a pain for me personally.
| # Run tests on main just so the module and build cache can be saved and used | ||
| # in PRs. This speeds up the time it takes to test PRs dramatically. |
There was a problem hiding this comment.
So the build cache of main is used when building PRs, but the build cache of one PR is not used when building other PRs? I thought that the cache would be something stored on a build server, and it'd be used regardless of which branch is being built.
There was a problem hiding this comment.
"I thought that the cache would be something stored on a build server, and it'd be used regardless of which branch is being built." -> I believe that this is correct. I think that the point is to always have a fresh cache available from the main branch that PRs can use when building for the first time
There was a problem hiding this comment.
When is this workflow ran on the main branch? Is it when a PR is merged to it? If the test already ran during the PR, I'm not sure if there is any point in having them also run after the PR is merged? I guess the cache would be the same.
There was a problem hiding this comment.
Hmm ok. I am not sure why this is, but if you could clarify in the comment in .github/workflows/integration-tests.yml that the cache will only be populated if we run this on main and not just the CI of any other branch? It'd be nice if @rfratto could help clarify further why this is, if he happens to remember the reason.
There was a problem hiding this comment.
I think we cant try without it and see how it behaves. If we see that the PRs don't hit the cache we can investigate further
There was a problem hiding this comment.
If we see that the PRs don't hit the cache
Do you know how to check for this? IS there a log line?
There was a problem hiding this comment.
If the tests run under 4 mins then the cache was hit, if it runs in around 14mins that means that it spent 10mins to download the dependencies instead of using the cache
github-actions
bot
added
the
frozen-due-to-age
PR Description
This PR introduces integration testing to the agent for flow.
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist