Skip to content

ci: verify the solution is correct #38

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

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Conversation

frostming
Copy link
Contributor

Signed-off-by: Frost Ming me@frostming.com

Signed-off-by: Frost Ming <me@frostming.com>
Signed-off-by: Frost Ming <me@frostming.com>
Comment on lines +7 to +16
CHALLENGES_DIR = Path(__file__).parent.parent / "challenges"
ALL_SOLUTIONS = list(CHALLENGES_DIR.glob("**/solution.py"))


@pytest.mark.parametrize("solution_file", ALL_SOLUTIONS, ids=lambda x: x.parent.name)
def test_solution_valid(solution_file: Path):
with solution_file.open() as f:
code = f.read()
result = ChallengeManager._type_check_with_pyright(code)
assert result.passed, result.stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should test these cases in parallel to reduce the total time of CI. How about pytest-xdist? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here you go

Signed-off-by: Frost Ming <me@frostming.com>
Copy link
Contributor

@100gle 100gle left a comment

Choose a reason for hiding this comment

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

LGTM!

with solution_file.open() as f:
code = f.read()
result = ChallengeManager._type_check_with_pyright(code)
assert result.passed, result.stdout
Copy link
Owner

Choose a reason for hiding this comment

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

Curious, is there a need to assert result.stdout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will give the typecheck output when the test fails, which helps up debug the solution.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, makes sense.

@laike9m
Copy link
Owner

laike9m commented Nov 10, 2023

Thanks, I always wanted to add this but don't have time. Overall LGTM.

Some new solutions were just added, do you mind do a pull and test them? Appreciate it!

@frostming
Copy link
Contributor Author

Some new solutions were just added, do you mind do a pull and test them? Appreciate it!

You can rebase the main onto the pull requests after this is merged. New solutions will be automatically picked up for CI, and the author will know the status.

@laike9m
Copy link
Owner

laike9m commented Nov 10, 2023

Some new solutions were just added, do you mind do a pull and test them? Appreciate it!

You can rebase the main onto the pull requests after this is merged. New solutions will be automatically picked up for CI, and the author will know the status.

I mean they've been added to the codebase
73fa81e

but let me merge this and run it myself.

@laike9m laike9m merged commit 0e1154e into laike9m:main Nov 10, 2023
@frostming frostming deleted the ci/test-solution branch November 10, 2023 08:29
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.

3 participants