Skip to content
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

Merged
merged 7 commits into from
Dec 2, 2023
Merged

feat: test file generator #11

merged 7 commits into from
Dec 2, 2023

Conversation

S-ecki
Copy link
Owner

@S-ecki S-ecki commented Dec 2, 2023

Integrates generation of test files. Usage is documented in readme and comments.

@S-ecki
Copy link
Owner Author

S-ecki commented Dec 2, 2023

@dumazy would appreciate a short review :)
I dont know why, but I couldnt add you as a reviewer 🤔

test('Part 1', () => expect(day.solvePart1(), _puzzleSolutionPart1));
test('Part 2', () => expect(day.solvePart2(), _puzzleSolutionPart2));
},
);
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Owner Author

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

Copy link
Contributor

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),
      );
    },
  );

Copy link
Owner Author

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;
Copy link
Contributor

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

Copy link
Owner Author

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 👍

@dumazy
Copy link
Contributor

dumazy commented Dec 2, 2023

LGTM, just want to hear your thoughts about the conditional skip in one of the earlier threads. Thanks for adding this! 👏

@S-ecki S-ecki merged commit eeb7c0c into main Dec 2, 2023
@S-ecki S-ecki deleted the feat-test-file-generator branch December 2, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants