Add unit and E2E tests #479
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
AgnesToulet
requested review from
a team,
juanicabanas and
lucychen-grafana
and removed request for
a team
| // It writes the diff file to /testdata if tests are run with SAVE_DIFF=true. | ||
| function compareImage(testName: string, responseBody: any): number { | ||
| const goldenFilePath = path.join(goldenFilesFolder, `${testName}.png`); | ||
| if (process.env['UPDATE_GOLDEN'] === 'true') { |
There was a problem hiding this comment.
We are running all test cases only to update the images and the compareImage has this side effect, which I don't think should be here. Maybe we can create a specific script to only update images instead of running all tests and call it from yarn test-update.
There was a problem hiding this comment.
What about updating the test-update command to:
"test-update": "cross-env UPDATE_GOLDEN=true jest -t=screenshot",
I'm worried that if we create a separate script, we will have a lot of duplicated code with the test. But I agree that we don't need to run all the tests so this would only run the screenshot test. We could do the same for the test-diff command.
There was a problem hiding this comment.
The update script should be a loop over a map with a request and writing the response to an image. I think it should be decoupled entirely of the test suite. Maybe sounds like duplicating because that is what primarily tests are, just a request and comparing the response.
The map could contain a URL (the one used to render a panel) and an image name.
However, this shouldn't be a blocker, maybe this is too much and we can move on with this PR that is a great start and refactor later if needed.
|
@evictorero Thanks a lot for your thorough review! I updated the PR according to your feedback. I also had an issue with the Drone pipeline so I added a docker-compose file and some instructions to run tests in a Drone-like environment locally so this is easier to troubleshoot. It should work as before but please do test again to ensure I didn't break anything on non-Windows environments. |
There was a problem hiding this comment.
Looks great! Left one comment to think about it.
I think it's ready for merge but only thing that bothers me a bit is the test output, when successful, it writes a lot of console.log messages and it confuses me because the tests are all successful.
Can we remove the output messages to avoid noise when all tests succeed?
|
I have hidden the logs when tests are successful 👍 For the update, I agree with you that it could be better, but I think it will make more sense to do it when we have more tests. |
Fixes #434