Conversation
SAN-MUYUN
left a comment
SAN-MUYUN
left a comment
There was a problem hiding this comment.
Thank you for your hard work to set this up! I think this is a good starting point for implementing E2E tests.
For the .sh and .yml files, they seem fine to me. Would be better to let @VikramGoyal23 or someone who knows github CI better to look through it as well.
Generally LGTM!
| """Test that progress reset works correctly after verify has run.""" | ||
| exercise_dir = exercises_dir / EXERCISE_NAME | ||
| res = runner.run(["progress", "reset"], cwd=exercise_dir) | ||
| # TODO: verify that the progress has actually been reset |
There was a problem hiding this comment.
I agree that it is fine for us to leave this for now. Are we going to implement E2E test for reset for all/most of the exercises? Some exercises are easier to reset, while some are more prone to regressions.
Then, if we want to test this behavior for all exercises, we will need to do test_download for all the exercises.
There was a problem hiding this comment.
We shouldn't test the behaviour of the reset command for individual exercises. E2E testing is not meant to catch all bugs, as they're expensive to run and we can't possibly test every possibility. They're mainly there to verify the basic behaviour of the command, to ensure no significant regressions are introduced.
| ["progress", "sync", "off"], cwd=exercises_dir, stdin_text="y\n" | ||
| ) | ||
| res_off.assert_success() | ||
| res_off.assert_stdout_contains("Successfully removed your remote sync") |
There was a problem hiding this comment.
Not very sure if it is necessary here, but would it be better to check that /progress folder is still present?
There was a problem hiding this comment.
For a start, I'll test the command output, but we can refactor in the future to test more in depth, just need to verify that this works properly first before introducing more thorough tests.
VikramGoyal23
left a comment
VikramGoyal23
left a comment
There was a problem hiding this comment.
The tests and workflow all LGTM, I agree with @SAN-MUYUN on the points raised, I only have a few questions about the tests for check.
3 participants
Fixes git-mastery/git-mastery#65
As per RFC: https://docs.google.com/document/d/101Qh5SiW-MZGy3DH0lLvn2lqRutxkTKbx01A8dNyI6o/edit?tab=t.0#heading=h.x8t43doyrgeb
Requires merging into main branch first before tests can run.
Things to do before merge:
e2e-testenvironmentGH_PATas environment secret and repository secret (requires duplicate steps unfortunately)