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

Use Rust's built-in support for integration tests #23

Open
emwalker opened this issue Sep 24, 2023 · 11 comments
Open

Use Rust's built-in support for integration tests #23

emwalker opened this issue Sep 24, 2023 · 11 comments

Comments

@emwalker
Copy link
Collaborator

emwalker commented Sep 24, 2023

Currently this project is compiling binaries for integration tests under src/bin. It turns out Rust has a mechanism for dealing with integration tests. More here.

Suggestion: create out a top-level tests directory and move the parser_test and tokenizer_test files over to it.

2023-09-24_07-40

@jaytaph
Copy link
Member

jaytaph commented Sep 24, 2023

👍

@emwalker
Copy link
Collaborator Author

Having trouble tracking down this test input. Do you have this in your personal working directory and ignored by Git?

@jaytaph
Copy link
Member

jaytaph commented Sep 24, 2023

This is the html5lib-test repo which can be found here. Maybe a message should be displayed on where to find the repo when it isn't detected or given as argument.

@emwalker
Copy link
Collaborator Author

emwalker commented Sep 24, 2023

Ah, gotcha. I wasn't aware of the html5lib-tests test suite. Whether you emit a usage warning or not will depend on whether you attempt to move this to a Rust integration test suite (the suggestion at the top of this issue).

Pros of going with this suggestion:

  • Runs automatically with the other tests and can be integrated in the CI loop, catching potential regressions that are introduced
  • Leverages existing test patterns in Rust instead of being a one-off test harness outside of the usual approach
  • Will presumably require html5lib-tests to be vendored somewhere in the repo (good, because then everyone gets the same results)

Cons of going with this suggestion:

  • Will not have the nicely formatted results
  • Will have to #[ignore] failing tests again
  • Will have to vendor html5lib-tests

Now that I've downloaded the html5lib-tests suite and run the tests, I can better see what they're intended to accomplish. I still vaguely lean towards using the Rust integration test pattern. But I can also see why it might not make sense in this case.

If you go with the suggestion above, no need to emit a warning because the external test data will be in the repo. If you decide to pass on the suggestion, the command-line warning might be a good idea, and perhaps a note in the README as to where to find the test data.

@jaytaph
Copy link
Member

jaytaph commented Sep 24, 2023

Does this mean we have to keep an updated version of html5lib tests in our repo, or can we use something like git submodules for this purpose? I don't have a real opinion on what would be the best course in this case.

@emwalker
Copy link
Collaborator Author

emwalker commented Sep 24, 2023

Does this mean we have to keep an updated version of html5lib tests in our repo

It could be updated periodically, whenever someone thinks to check. But between updates, you'd have a stable target to develop against. So I'm assuming updates could be infrequent (e.g., once every few months or years, or whenever someone wants to).

Looks like the license for html5lib-tests is MIT, so I'm thinking vendoring won't present legal issues. I can look into what migrating the two test harnesses might look like if you like.

The vendored files would live under tests/data/html5lib-tests or something like that. I don't think Git submodules would be needed at first (if ever).

@jaytaph
Copy link
Member

jaytaph commented Sep 24, 2023

ok.. let's try and see how the vendoring would work. I'm not 100% happy with having 3rd party code inside the repo, but I think it will be the best course as you stated. I'm happy to see if we can get something up and running in a more rust-like test setup.

@emwalker
Copy link
Collaborator Author

Submitted #27

@emwalker
Copy link
Collaborator Author

@jaytaph should I migrate src/bin/parser_test.rs over to the new integration test pattern, or hold off for you to assess whether you like it first?

@jaytaph
Copy link
Member

jaytaph commented Sep 26, 2023

Let's wait for now. It seems a relative easy process to move this, but since the parser is still a big work in progress (making sure all tests pass takes a lot of time :( ), it would be easier at least for me to run the tests with more context (for instance, i'm currently displaying element stack dumps on each iteration, as this is hard for me to view in the step debugger). Moving the test might interface with the current parser branch. But let's keep this issue open and try to move it when the parser passes the most tests.

@emwalker
Copy link
Collaborator Author

Ok, that works.

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

No branches or pull requests

2 participants