-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: test file generator #11
Conversation
@dumazy would appreciate a short review :) |
test('Part 1', () => expect(day.solvePart1(), _puzzleSolutionPart1)); | ||
test('Part 2', () => expect(day.solvePart2(), _puzzleSolutionPart2)); | ||
}, | ||
); |
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.
Should we keep the Puzzle Input
group commented out by default to prevent the tests from failing?
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.
Else the test suite would still fail until you provide the _puzzleSolutionPart1
and _puzzleSolutionPart2
variables and make it less clear that your solution worked.
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.
That's a good point! Pushed the changes
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.
We could be a bit more "smart" about it and skip the test if the solution hasn't been filled in:
const _puzzleSolutionPart1 = null;
const _puzzleSolutionPart2 = null;
group(
'Day 01 - Puzzle Input',
() {
final day = Day01();
test(
'Part 1',
skip: _puzzleSolutionPart1 == null
? 'Skipped because _puzzleSolutionPart1 is null'
: false,
() => expect(day.solvePart1(), _puzzleSolutionPart1),
);
test(
'Part 1',
skip: _puzzleSolutionPart2 == null
? 'Skipped because _puzzleSolutionPart2 is null'
: false,
() => expect(day.solvePart1(), _puzzleSolutionPart2),
);
},
);
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.
That is perfect! I will add and commit it! 🚀
Was also thinking about skipping the tests without touching the actual test code, but stopped at the thought of a shouldTest
boolean set by the user 😅
Thanks a lot! 💙
\'''; | ||
|
||
/// The solution for the FIRST PART's example, which is given by the puzzle. | ||
const _exampleSolutionPart1 = 0; |
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.
Should this be a different value than 0
, Like -1
or null
? The tests would pass by default since the default return value is 0
as well. Not sure if this is the desired behaviour
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.
This would indeed pass at the start, which might not be great from a purist POV.
But as this will not pass as soon as the user adds the actual solution from the example (or adds his solution) - and I expect them to do this when they use the test - I think it is ok.
An even bigger case for me to leave it as is: The examplePart2 will not be distracting/annoying (red 😅) until you acutally start to work at it :)
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.
Good point 👍
avoid false negatives
LGTM, just want to hear your thoughts about the conditional |
Integrates generation of test files. Usage is documented in readme and comments.