-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CC-753 upgrade tester utils #45
Conversation
CC-753 Upgrade tester_utils everywhere
To support extensions, we've had to make a breaking change to We need to migrate over all course testers (and the "tester-template") to use this new structure. Here's an example PR that does the migration for Redis: codecrafters-io/redis-tester#37. The description in that PR contains a rough list of steps that need to be taken:
The list of repositories that use tester-utils can be found here: https://github.com/codecrafters-io/tester-utils/network/dependents. |
tester_utils_testing.TestTesterOutput(t, testerDefinition, testCases) | ||
} | ||
|
||
func normalizeTesterOutput(testerOutput []byte) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that the CompareOutputWithFixture
function in tester utils requires a NormalizeOutputFunc
. is it better that this is always required? not sure how to normalize output for git, so i just returned the output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's okay to just return the output if that works - some challenges will have randomness involved (like with TCP port numbers), that's where the normalization function is useful.
there aren't tests for some of the stages. should i make a linear test for that for each tester or no |
appreciate if you could take a look at one instance before doing the other testers |
Nope, it's fine - we'd want to retain whatever tests we have, don't need to add new ones as part of this migration.
@libmartinito you've got failing tests - other than that looks good. Does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(comment added)
yep it works locally. the problem was the randomness in the file names, contents, hashed values. updated to be more deterministic. |
Updated to get tests to pass
@libmartinito the randomness in tests is intentional. We do this to prevent users from accidentally passing the stage by hardcoding values. Let's find another way around it - check what other challenges do |
No description provided.