Moved tokenizer.rs and tree_construction.rs tests into crates for issue #445 #457

kaigidwani · 2024-05-03T18:47:31Z

For issue #445, I have copied the tokenizer.rs over into crates/gosub_html5/src/tokenizer as test.rs. I have also moved the contents of tree_construction.rs onto the end of crates/gosub_html5/src/parser/tree_builder.rs. I made sure to add the module declarations and #[cfg(test)] above them.

Moved tokenizer.rs from tests to its crate. Also added the #[cfg(test)] and mod declaration as requested.

It was copied into its crate.

Copied tree_construction.rs from tests into its specific crate. I also put it in the module as requested.

Deleted tree_construction.rs from the test folder as it was copied over to a crate

Sharktheone · 2024-05-03T21:07:30Z

crates/gosub_html5/src/tokenizer/test.rs

+#[cfg(test)]
+mod tests{


I don't think an extra tests module is necessary here. You already made a module by adding the file. So, I guess rename the file to tests.rs and add #[cfg(test) over the module definition in tokenizer.rs (which is missing btw)

Removes the #[cfg(test)] from the file and removed extra indentation. Also renamed it to tests.rs as asked.

Sharktheone

Now only the module declaration is missing. You need to add

#[cfg(test)
mod tests

to crates/gosub_html5/src/tokenizer.rs under line 6. E.g. here: https://github.com/gosub-browser/gosub-engine/blob/99d8f224649772511d392f396ca8e010c102ee7d/crates/gosub_html5/src/tokenizer.rs#L1-L6

Sharktheone

LGTM. Now we only need to look at what CI says

MoralCode · 2024-05-05T17:57:24Z

crates/gosub_html5/src/parser/tree_builder.rs

+    use gosub_testing::testing::tree_construction::Harness;
+    use test_case::test_case;


It seems to not like these imports for some reason when i run make test locally. Where is the test_case macro being defined?

It's an external crate. Basically just run

cargo add test_case --dev -p gosub_html5

It also seems like fmt and clippy aren't happy, it can be fixed via make format

I haven't looked really into what is wrong, again I'll do that tomorrow.

kaigidwani · 2024-05-05T17:59:28Z

I'm seeing this in the CI logs too:
error: couldn't read tests/tree_construction.rs: No such file or directory (os error 2)
Any suggestions on how to fix this?

Sharktheone · 2024-05-05T21:52:46Z

Oh, yeah... I started the test run and then completely forgot about it... I'll take a look tomorrow.

Sharktheone · 2024-05-06T06:23:54Z

Okay, looks like, the old module definition is still there. Just delete this file:
https://github.com/gosub-browser/gosub-engine/blob/c6e81c66e90e886bacf82eff35d44d8bd94da827/tests/lib.rs#L1-L5

and in Cargo.toml these lines

https://github.com/gosub-browser/gosub-engine/blob/c6e81c66e90e886bacf82eff35d44d8bd94da827/Cargo.toml#L21-L27

MoralCode · 2024-05-06T13:50:24Z

Looks like I also had to do cargo add --path ./crates/gosub_testing --dev -p gosub_html5 to get some additional similar rust errors to go away, but then it seemed to mostly work (other than some tests failing because they cant seem to find the requisite data files). To rerun just the tests that arent working, rust was instructing me to run cargo test -p gosub_html5 --lib in case that helps speed up debugging

MoralCode · 2024-05-06T14:20:09Z

looks like the testing data files for html5lib-tests also need to be moved since it seems like the test fixtures are looking for them in ./tests/data/html5lib-tests which is relative to the crate root (which got moved into the gosub_html5 crate as part of this PR.

Deleted the [[test]] lines.

Sharktheone · 2024-05-06T18:21:26Z

Okay, so let's just run CI again and see what fails...

Sharktheone · 2024-05-06T18:24:16Z

So, if you want to speed things up, I can push a commit to fix the rest of the checks. I don't know if you want that, since from as far as I know, this is in context of some sort of class.

Sharktheone · 2024-05-06T18:37:05Z

So, here is what is missing:

In crates/gosub_html5/Cargo.toml (just appended to the bottom)

[dev-dependencies]
test-case = "3.3.1"
gosub_testing = { path = "../gosub_testing", features = [] }

afterward, change the FIXTURE_ROOT in crates/gosub_testing/src/testing.rs to `../../tests/data/html5lib-tests

Here is the complete diff

diff --git a/Cargo.lock b/Cargo.lock
index 7d8f68a..0c729b3 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1324,9 +1324,11 @@ dependencies = [
  "derive_more",
  "gosub_css3",
  "gosub_shared",
+ "gosub_testing",
  "lazy_static",
  "log",
  "phf",
+ "test-case",
  "thiserror",
  "ureq",
  "url",
diff --git a/crates/gosub_html5/Cargo.toml b/crates/gosub_html5/Cargo.toml
index a06f337..22b9e39 100644
--- a/crates/gosub_html5/Cargo.toml
+++ b/crates/gosub_html5/Cargo.toml
@@ -20,3 +20,8 @@ log = { version = "0.4.21", features = [] }
 [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
 ureq = "2.9.7"
 
+
+[dev-dependencies]
+test-case = "3.3.1"
+gosub_testing = { path = "../gosub_testing", features = [] }
+
diff --git a/crates/gosub_testing/src/testing.rs b/crates/gosub_testing/src/testing.rs
index e0deb4b..3bd5bd5 100644
--- a/crates/gosub_testing/src/testing.rs
+++ b/crates/gosub_testing/src/testing.rs
@@ -2,5 +2,5 @@
 pub mod tokenizer;
 pub mod tree_construction;
 
-pub const FIXTURE_ROOT: &str = "./tests/data/html5lib-tests";
+pub const FIXTURE_ROOT: &str = "../../tests/data/html5lib-tests";
 pub const TREE_CONSTRUCTION_PATH: &str = "tree-construction";

lastly, run cargo fmt --all in the root of the project.

You can push it in one commit if you want.

MoralCode · 2024-05-06T18:50:36Z

So, if you want to speed things up, I can push a commit to fix the rest of the checks. I don't know if you want that, since from as far as I know, this is in context of some sort of class.

yep it is for a class! As the class TA i was planning to commit the rest of the fixes after the due date (maybe later this week) unless @kaigidwani wants to give it a shot.

MoralCode · 2024-05-06T18:51:40Z

afterward, change the FIXTURE_ROOT in crates/gosub_testing/src/testing.rs to `../../tests/data/html5lib-tests

i ended up just moving the data files to the crate since it seems like FIXTURE_ROOT is relative to the crate anyway. Would you prefer one way or the other for solving this?

kaigidwani · 2024-05-06T18:57:56Z

Yes if possible I would like to give it a shot, but I wouldn't be able to get to this until Thursday, just because of how my schedule is working out with finals season. T-T
I appreciate you guys letting me take on this issue even though you both clearly know how to do it easily haha

Sharktheone · 2024-05-06T19:00:18Z

afterward, change the FIXTURE_ROOT in crates/gosub_testing/src/testing.rs to `../../tests/data/html5lib-tests

i ended up just moving the data files to the crate since it seems like FIXTURE_ROOT is relative to the crate anyway. Would you prefer one way or the other for solving this?

I actually don't really care, but it would keep test-files in one directory, however as you said, the path is relative to the crate, which is a bit messy... Do it like you want. If you have the solution with moving the test files already, you can also use that.

Sharktheone · 2024-05-06T19:02:31Z

Yes if possible I would like to give it a shot, but I wouldn't be able to get to this until Thursday, just because of how my schedule is working out with finals season. T-T I appreciate you guys letting me take on this issue even though you both clearly know how to do it easily haha

Yeah, no problem, you can only learn things when making your hands dirty, and since you can't break anything, I don't see there any reason why not letting you (or others) doing that. When I think about my starts of coding, I don't think it was any better 😉

kaigidwani · 2024-05-10T02:58:15Z

Hey, sorry to go back on what I said, but my plans are getting more complicated and I'd like to take some time off before I have to start my Summer job. Since you already have that push would it be alright if you went through with it? Thanks so much for the help with this, I really appreciate it.

Sharktheone · 2024-05-10T12:23:01Z

Yup, I can do that

MoralCode · 2024-05-10T19:57:26Z

@Sharktheone just pushed my commits to hopefully finish up this PR - currently rerunning the tests locally to double check they still work.

I figure since the tests and these data files are linked using a macro, it may be best to keep the data as close to the functions being tested as possible - similar to how it may be beneficial to keep the tests close to the code so that they are more likely to remain up to date.

Sharktheone · 2024-05-10T20:52:32Z

Ah, thanks, looks good

Sharktheone · 2024-05-10T21:32:45Z

Okay, somehow I can't push to the PR branch, neither over a new remote nor over the GitHub ref, maybe because it's the main branch?

So these are my changes to get also the benchmarks working

diff --git a/Cargo.lock b/Cargo.lock
index 0c729b3..e60b7be 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1321,6 +1321,7 @@ dependencies = [
 name = "gosub_html5"
 version = "0.1.0"
 dependencies = [
+ "criterion",
  "derive_more",
  "gosub_css3",
  "gosub_shared",
diff --git a/Cargo.toml b/Cargo.toml
index 8b5692d..087e20b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,14 +18,6 @@ members = [
 [[example]]
 name = "html5-parser"
 
-[[bench]]
-name = "tokenizer"
-harness = false
-
-[[bench]]
-name = "tree_construction"
-harness = false
-
 [[bench]]
 name = "tree_iterator"
 harness = false
diff --git a/crates/gosub_html5/Cargo.toml b/crates/gosub_html5/Cargo.toml
index 9cfa992..8cb481e 100644
--- a/crates/gosub_html5/Cargo.toml
+++ b/crates/gosub_html5/Cargo.toml
@@ -23,4 +23,13 @@ ureq = "2.9.7"
 [dev-dependencies]
 gosub_testing = { path = "../gosub_testing" }
 test-case = "3.3.1"
+criterion = { version = "0.5.1", features = ["html_reports"] }
 
+
+[[bench]]
+name = "tokenizer"
+harness = false
+
+[[bench]]
+name = "tree_construction"
+harness = false
diff --git a/benches/tokenizer.rs b/crates/gosub_html5/benches/tokenizer.rs
similarity index 100%
rename from benches/tokenizer.rs
rename to crates/gosub_html5/benches/tokenizer.rs
diff --git a/benches/tree_construction.rs b/crates/gosub_html5/benches/tree_construction.rs
similarity index 100%
rename from benches/tree_construction.rs
rename to crates/gosub_html5/benches/tree_construction.rs

MoralCode · 2024-05-11T00:23:24Z

There you go! TIL you can just copy paste the diff into a text file and git apply it

Sharktheone · 2024-05-11T00:29:23Z

Interesting... looks like it somehow didn't apply the

diff --git a/Cargo.toml b/Cargo.toml
index 8b5692d..087e20b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,14 +18,6 @@ members = [
 [[example]]
 name = "html5-parser"
 
-[[bench]]
-name = "tokenizer"
-harness = false
-
-[[bench]]
-name = "tree_construction"
-harness = false
-
 [[bench]]
 name = "tree_iterator"
 harness = false

part from the diff

Cargo.toml

MoralCode · 2024-05-11T22:50:29Z

Done

Sharktheone · 2024-05-11T23:02:04Z

Okay, looks good. Looks like the last problem is commit signing.

MoralCode · 2024-05-14T02:37:46Z

Okay, looks good. Looks like the last problem is commit signing.

Is there a particular reason this is required? I couldn't see much in CONTRIBUTING.md and (as far as I've read at least) it seems like the linux kernel (and the OSS community in general) tends to prefer tag signing over commit signing

Sharktheone · 2024-05-14T11:27:46Z

Okay, looks good. Looks like the last problem is commit signing.

Is there a particular reason this is required? I couldn't see much in CONTRIBUTING.md and (as far as I've read at least) it seems like the linux kernel (and the OSS community in general) tends to prefer tag signing over commit signing

I don't know, why have set this up to block merging with unsigned commits, maybe @jaytaph can say anything about that.

This is how it looks on my side

MoralCode · 2024-05-14T14:08:16Z

it also seems like theres some issues with the check_authors CI job:

Seems like it's:

not detecting Kai as a contributor
pulling Sharks name into the email address field for me
failing to post a comment to the PR with the requested changes

Sharktheone · 2024-05-14T21:18:00Z

Hmm, that's interesting. I have no clue why that's happening... I guess, just add yourself to the AUTHORS file and call it a day?

jaytaph · 2024-05-15T10:03:34Z

Don't worry about the authors file. The check isn't that good, and should be either be changed or even removed (and maybe there are other - better - actions available for this. I haven't done the research).

As far as signed commits: they are preferred to make sure that code committed is signed off by that specific user. Even though everybody can commit, we should make sure that it's committed by the same user. For instance, There could come PR that looks like (co-)authored by one of the core members even though it's written by "evil hacker" that includes a backdoor (fortunately, that never happens /s).

So with signed commits, we assure that that commit really comes from that specific user. This becomes more important with many contributers and PRs.

Btw, signed tags will be a thing later on as soon as we actually have taggable code :)

MoralCode · 2024-05-27T15:24:48Z

finally figured out commit signing

Sharktheone · 2024-05-27T15:29:18Z

Okay, I'm able to merge this now! 🎉

kaigidwani added 4 commits May 3, 2024 14:32

Moved tokenizer.rs from tests to crates

67c1ea7

Moved tokenizer.rs from tests to its crate. Also added the #[cfg(test)] and mod declaration as requested.

Deleted tokenizer.rs from tests folder.

9393b48

It was copied into its crate.

Copied tree_construction.rs from tests into a crate.

d290386

Copied tree_construction.rs from tests into its specific crate. I also put it in the module as requested.

Deleted tree_construction.rs from the test folder

43fca24

Deleted tree_construction.rs from the test folder as it was copied over to a crate

Sharktheone requested changes May 3, 2024

View reviewed changes

Sharktheone self-assigned this May 3, 2024

Sharktheone added the tests Anything related to tests label May 3, 2024

Update and rename test.rs to tests.rs

665c762

Removes the #[cfg(test)] from the file and removed extra indentation. Also renamed it to tests.rs as asked.

Sharktheone requested changes May 5, 2024

View reviewed changes

Update tokenizer.rs

2b042a2

Sharktheone approved these changes May 5, 2024

View reviewed changes

MoralCode reviewed May 5, 2024

View reviewed changes

kaigidwani added 2 commits May 6, 2024 11:03

Delete tests/lib.rs

ee44864

Update Cargo.toml

8fc2ccb

Deleted the [[test]] lines.

Sharktheone reviewed May 11, 2024

View reviewed changes

Cargo.toml Show resolved Hide resolved

MoralCode added 6 commits May 27, 2024 11:18

cargo add test_case --dev -p gosub_html5

272c205

make format

c0c543a

import gosub testing in html5 crate

4a819c3

move testing data

50af07b

apply diff from PR to fix final changes

c1ee0fa

remove benchmarks that the other patch didnt get

693eced

MoralCode force-pushed the main branch from bc4ea3c to 693eced Compare May 27, 2024 15:20

Sharktheone merged commit f218de8 into gosub-io:main May 27, 2024
20 of 21 checks passed

Sharktheone mentioned this pull request May 31, 2024

Move "global" tests from ./tests to the correct crates #445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved tokenizer.rs and tree_construction.rs tests into crates for issue #445 #457

Moved tokenizer.rs and tree_construction.rs tests into crates for issue #445 #457

kaigidwani commented May 3, 2024

Sharktheone May 3, 2024

Sharktheone left a comment

Sharktheone left a comment

MoralCode May 5, 2024 •

edited

Loading

Sharktheone May 5, 2024

kaigidwani commented May 5, 2024 •

edited

Loading

Sharktheone commented May 5, 2024

Sharktheone commented May 6, 2024

MoralCode commented May 6, 2024 •

edited

Loading

MoralCode commented May 6, 2024

Sharktheone commented May 6, 2024

Sharktheone commented May 6, 2024

Sharktheone commented May 6, 2024

MoralCode commented May 6, 2024 •

edited

Loading

MoralCode commented May 6, 2024

kaigidwani commented May 6, 2024

Sharktheone commented May 6, 2024

Sharktheone commented May 6, 2024

kaigidwani commented May 10, 2024

Sharktheone commented May 10, 2024

MoralCode commented May 10, 2024

Sharktheone commented May 10, 2024

Sharktheone commented May 10, 2024

MoralCode commented May 11, 2024

Sharktheone commented May 11, 2024

MoralCode commented May 11, 2024

Sharktheone commented May 11, 2024

MoralCode commented May 14, 2024 •

edited

Loading

Sharktheone commented May 14, 2024

MoralCode commented May 14, 2024

Sharktheone commented May 14, 2024

jaytaph commented May 15, 2024

MoralCode commented May 27, 2024

Sharktheone commented May 27, 2024

		use gosub_testing::testing::tree_construction::Harness;
		use test_case::test_case;

Moved tokenizer.rs and tree_construction.rs tests into crates for issue #445 #457

Moved tokenizer.rs and tree_construction.rs tests into crates for issue #445 #457

Conversation

kaigidwani commented May 3, 2024

Sharktheone May 3, 2024

Choose a reason for hiding this comment

Sharktheone left a comment

Choose a reason for hiding this comment

Sharktheone left a comment

Choose a reason for hiding this comment

MoralCode May 5, 2024 • edited Loading

Choose a reason for hiding this comment

Sharktheone May 5, 2024

Choose a reason for hiding this comment

kaigidwani commented May 5, 2024 • edited Loading

Sharktheone commented May 5, 2024

Sharktheone commented May 6, 2024

MoralCode commented May 6, 2024 • edited Loading

MoralCode commented May 6, 2024

Sharktheone commented May 6, 2024

Sharktheone commented May 6, 2024

Sharktheone commented May 6, 2024

MoralCode commented May 6, 2024 • edited Loading

MoralCode commented May 6, 2024

kaigidwani commented May 6, 2024

Sharktheone commented May 6, 2024

Sharktheone commented May 6, 2024

kaigidwani commented May 10, 2024

Sharktheone commented May 10, 2024

MoralCode commented May 10, 2024

Sharktheone commented May 10, 2024

Sharktheone commented May 10, 2024

MoralCode commented May 11, 2024

Sharktheone commented May 11, 2024

MoralCode commented May 11, 2024

Sharktheone commented May 11, 2024

MoralCode commented May 14, 2024 • edited Loading

Sharktheone commented May 14, 2024

MoralCode commented May 14, 2024

Sharktheone commented May 14, 2024

jaytaph commented May 15, 2024

MoralCode commented May 27, 2024

Sharktheone commented May 27, 2024

MoralCode May 5, 2024 •

edited

Loading

kaigidwani commented May 5, 2024 •

edited

Loading

MoralCode commented May 6, 2024 •

edited

Loading

MoralCode commented May 6, 2024 •

edited

Loading

MoralCode commented May 14, 2024 •

edited

Loading