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.

Sign up for GitHub

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

Jump to bottom

feat(parser): exit when html5lib-tests dir is missing #32

Merged
merged 2 commits into from
Oct 1, 2023
Merged

feat(parser): exit when html5lib-tests dir is missing #32

merged 2 commits into from
Oct 1, 2023

Conversation

neuodev
Copy link
Collaborator

@neuodev neuodev commented Sep 25, 2023

No description provided.

@jaytaph
Copy link
Member

jaytaph commented Sep 25, 2023

Thanks for the PR. I'll merge it for now, but we are in the process of moving these tests outside the bin directory and have them as integrated rust tests (also, we vendored the html5lib directory so it will always be present). This file might be removed soon, but I appreciate the help!

@jaytaph
Copy link
Member

jaytaph commented Sep 25, 2023

ow, I see we the commits are not signed. Could you rebase your commits using signing? See Learn more about signing commits.

@emwalker
Copy link
Collaborator

@neuodev note that html5lib-tests is now vendored under tests/data/html5lib-tests.

@neuodev neuodev force-pushed the ahmed/html5lib-tests-dir branch 3 times, most recently from 59f79be to a560460 Compare September 26, 2023 11:12
@neuodev
Copy link
Collaborator Author

neuodev commented Sep 26, 2023

ow, I see we the commits are not signed. Could you rebase your commits using signing? See Learn more about signing commits.

Got it! moving on all of my commit messages will be signed. I like the badge 🙂

@neuodev
Copy link
Collaborator Author

neuodev commented Sep 26, 2023

@neuodev note that html5lib-tests is now vendored under tests/data/html5lib-tests.

I see! but when are we cloning it from GitHub?

@jaytaph
Copy link
Member

jaytaph commented Sep 26, 2023

The html5lib-tests repo is copy/pasted into our own repository in /tests/data/html5lib-tests. We will try and keep this in sync as much as possible, but because we copy it instead of linking it, we can always create a deterministic result from the tests (an update on html5lib-test could break our builds).

So we can simply point to the tests/data/html5lib-tests directory in case no directory has been given (they might want to, for instance, to test against a new html5lib-test repository for instance)

@jaytaph
Copy link
Member

jaytaph commented Sep 26, 2023

So maybe this PR could include that we are not defaulting to ./html5lib-tests, but the ./tests/data/html5lib-tests directory instead? (and worst-case give a failure if that directory is not found)

@emwalker
Copy link
Collaborator

So maybe this PR could include that we are not defaulting to ./html5lib-tests, but the ./tests/data/html5lib-tests directory instead? (and worst-case give a failure if that directory is not found)

I think I'd take this approach for now. Something like, point parser_test to the vendored library, not add html5lib-tests to .gitignore and drop the optional argument from the parser_test binary.

@neuodev
Copy link
Collaborator Author

neuodev commented Sep 26, 2023

So maybe this PR could include that we are not defaulting to ./html5lib-tests, but the ./tests/data/html5lib-tests directory instead? (and worst-case give a failure if that directory is not found)

I think I'd take this approach for now. Something like, point parser_test to the vendored library, not add html5lib-tests to .gitignore and drop the optional argument from the parser_test binary.

Agree! I will apply the changes!

@neuodev
feat(parser): exit when html5lib-tests dir is missing
cf42952
@neuodev
chore(parser): edit printed test file path
24cebc5 
Without this change we will not be able to inspect the test at the exact
location in the test files.

This is now possible by using single semicolon instead of two to be able to open
the test files in vscode at the test line
@neuodev
Copy link
Collaborator Author

neuodev commented Oct 1, 2023

@jaytaph I implemented the changes pointed above! this PR is read for review!

@jaytaph
Copy link
Member

jaytaph commented Oct 1, 2023

@neuodev Thanks for the PR!

@jaytaph jaytaph merged commit 2964b89 into gosub-io:main Oct 1, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ernest-rudnicki ernest-rudnicki ernest-rudnicki left review comments

@jaytaph jaytaph jaytaph approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@neuodev @jaytaph @emwalker @ernest-rudnicki