DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Create node version of gentest script #1498

Closed
joevilches wants to merge 1 commits from export-D51874433 into main
pull from: export-D51874433
merge into: DaddyFrosty:main
Conversation 8 Commits 1 Files Changed 4 +118
joevilches commented 2023-12-07 13:05:28 -08:00 (Migrated from github.com)
Copy Link

Summary:
The only instance of ruby in this repository is gentest.rb used to generate test cases from html fixtures. This is quite annoying as ruby is not the most popular compared to something like Node and it does not integrate into the rest of our stack. I changed this to use Node.js instead. Instead of watir we now use selenium-webdriver. watir is backed by Selenium so I do not expect anything to change.

Next commits will add command line options, clean up gentest.rb and its references, and change the README

Differential Revision: D51874433

Summary: The only instance of ruby in this repository is `gentest.rb` used to generate test cases from html fixtures. This is quite annoying as ruby is not the most popular compared to something like Node and it does not integrate into the rest of our stack. I changed this to use Node.js instead. Instead of `watir` we now use `selenium-webdriver`. `watir` is backed by Selenium so I do not expect anything to change. Next commits will add command line options, clean up gentest.rb and its references, and change the README Differential Revision: D51874433
facebook-github-bot commented 2023-12-07 13:05:39 -08:00 (Migrated from github.com)
Copy Link

This pull request was exported from Phabricator. Differential Revision: D51874433

This pull request was **exported** from Phabricator. Differential Revision: [D51874433](https://www.internalfb.com/diff/D51874433)
nicoburns commented 2023-12-10 15:46:10 -08:00 (Migrated from github.com)
Copy Link

If you use playwright-core instead of selenium-webdriver then it will manage browser versions for you, and would also remove the need for users to install chromedriver.

If you use playwright-core instead of selenium-webdriver then it will manage browser versions for you, and would also remove the need for users to install chromedriver.
👍 1
nicoburns (Migrated from github.com) reviewed 2023-12-13 06:45:34 -08:00
nicoburns (Migrated from github.com) left a comment
Copy Link

Excited to see this new runner. I am very much considering adopting it for Taffy. Partly because it's a lot faster than our runner (which has to compile a Rust script before it runs, and runs each fixture in a fresh browser tab rather than batching them). And partly because there are a bunch of other derivatives of Yoga/Taffy (mostly ports/bindings to other languages) that could potentially use this as well, and the main benefit of our runner (that it is written in Rust, the same as our library) doesn't seem as beneficial in that context.

I've left you a few review comments :)

(CC: @alice-i-cecile)

Excited to see this new runner. I am very much considering adopting it for Taffy. Partly because it's a lot faster than our runner (which has to compile a Rust script before it runs, and runs each fixture in a fresh browser tab rather than batching them). And partly because there are a bunch of other derivatives of Yoga/Taffy (mostly ports/bindings to other languages) that could potentially use this as well, and the main benefit of our runner (that it is written in Rust, the same as our library) doesn't seem as beneficial in that context. I've left you a few review comments :) (CC: @alice-i-cecile)
gentest/gentest-driver.ts
@@ -0,0 +1,89 @@
/**
nicoburns (Migrated from github.com) commented 2023-12-13 06:15:34 -08:00
Copy Link

This script needs instructions on how to run it (especially given that it's a TS file that won't run with vanilla node):

  • Minimum node version
  • That you need to run NPM install
  • That you need chromedriver (if you do - I already had it installed so can't be sure)
  • The run command (node --loader babel-register-esm gentest-driver.ts ?). Perhaps this could be included in the package.json as a script. And then the instructions could just be npm run gentest or similar.
This script needs instructions on how to run it (especially given that it's a TS file that won't run with vanilla node): - Minimum node version - That you need to run NPM install - That you need chromedriver (if you do - I already had it installed so can't be sure) - The run command (`node --loader babel-register-esm gentest-driver.ts` ?). Perhaps this could be included in the package.json as a script. And then the instructions could just be `npm run gentest` or similar.
gentest/gentest-driver.ts
@@ -0,0 +34,4 @@
process.chdir(path.dirname(process.argv[1]));
const fixtures = await fs.readdir('./fixtures');
for (const fileName of fixtures) {
nicoburns (Migrated from github.com) commented 2023-12-13 06:04:24 -08:00
Copy Link

It would be nice if the behaviour when the generation script (script run within the browser tab) throws an error could be improved. Specifically, it would be nice if no tests were updated in that case (and ideally that the error, along with the file/fixture that caused it, was printed to the console).

It would be nice if the behaviour when the generation script (script run within the browser tab) throws an error could be improved. Specifically, it would be nice if no tests were updated in that case (and ideally that the error, along with the file/fixture that caused it, was printed to the console).
gentest/gentest-driver.ts
@@ -0,0 +40,4 @@
console.log('Generate', fileNameNoExtension);
// TODO: replace this with something more robust than just blindly replacing
// start/end in the entire fixture
nicoburns (Migrated from github.com) commented 2023-12-13 06:37:09 -08:00
Copy Link

Probably the fixtures should be changed to use inset-inline-start and inset-inline-end instead of left/right? I don't think those existed when these tests were created.

Probably the fixtures should be changed to use `inset-inline-start` and `inset-inline-end` instead of left/right? I don't think those existed when these tests were created.
gentest/gentest-driver.ts
@@ -0,0 +65,4 @@
await fs.writeFile(
`../tests/generated/${fileNameNoExtension}.cpp`,
eval(logs[0].message.replace(/^[^"]*/, '')),
nicoburns (Migrated from github.com) commented 2023-12-13 06:20:50 -08:00
Copy Link

I believe this could use JSON.parse instead of eval (which would probably be preferable both from a security and code readability perspective?)

I believe this could use `JSON.parse` instead of `eval` (which would probably be preferable both from a security and code readability perspective?)
gentest/gentest-driver.ts
@@ -0,0 +66,4 @@
await fs.writeFile(
`../tests/generated/${fileNameNoExtension}.cpp`,
eval(logs[0].message.replace(/^[^"]*/, '')),
);
nicoburns (Migrated from github.com) commented 2023-12-13 06:06:41 -08:00
Copy Link

Looks like this doesn't delete old tests where the test fixture has been deleted or renamed? I think that was being handled in the old script by deleting the entire directory prior to writing out the new tests. IMO the best order would be:

  • Run all fixtures in the browser (keeping results in memory)
  • If all run without error:
    • Delete existing test directories
    • Write out all tests in parallel
Looks like this doesn't delete old tests where the test fixture has been deleted or renamed? I think that was being handled in the old script by deleting the entire directory prior to writing out the new tests. IMO the best order would be: - Run all fixtures in the browser (keeping results in memory) - If all run without error: - Delete existing test directories - Write out all tests in parallel
joevilches (Migrated from github.com) reviewed 2023-12-13 12:05:01 -08:00
gentest/gentest-driver.ts
@@ -0,0 +34,4 @@
process.chdir(path.dirname(process.argv[1]));
const fixtures = await fs.readdir('./fixtures');
for (const fileName of fixtures) {
joevilches (Migrated from github.com) commented 2023-12-13 12:05:01 -08:00
Copy Link

A followup me an @NickGerleman were thinking about was to make it so that there is minimum logic being run on the browser. Right now that is where all the complicated code generation comes from because that is where we inspect the browsers layout. Ideally we port that over to being driven off of the browser if our driver library has such capabilities (not very familiar with all that you can do). Right now, because all of that logic is on the browser and we just read the console.logs, it is hard to do anything elegant around error handling and debugging.

A followup me an @NickGerleman were thinking about was to make it so that there is minimum logic being run on the browser. Right now that is where all the complicated code generation comes from because that is where we inspect the browsers layout. Ideally we port that over to being driven off of the browser if our driver library has such capabilities (not very familiar with all that you can do). Right now, because all of that logic is on the browser and we just read the console.logs, it is hard to do anything elegant around error handling and debugging.
joevilches (Migrated from github.com) reviewed 2023-12-13 12:06:24 -08:00
gentest/gentest-driver.ts
@@ -0,0 +66,4 @@
await fs.writeFile(
`../tests/generated/${fileNameNoExtension}.cpp`,
eval(logs[0].message.replace(/^[^"]*/, '')),
);
joevilches (Migrated from github.com) commented 2023-12-13 12:06:24 -08:00
Copy Link

ooo good callout. I can take a look at that

ooo good callout. I can take a look at that
joevilches (Migrated from github.com) reviewed 2023-12-13 12:09:33 -08:00
gentest/gentest-driver.ts
@@ -0,0 +1,89 @@
/**
joevilches (Migrated from github.com) commented 2023-12-13 12:09:33 -08:00
Copy Link

Yeah in some of the later commits in this stack I update the readme for instructions on how to get this to work wrt to dependencies. We also have scripts in place: yarn gentest and yarn gentest-validate which abstracts that out that loader.

As for chromedriver, according to https://www.selenium.dev/documentation/webdriver/troubleshooting/errors/driver_location/#use-the-latest-version-of-selenium, it appears that selenium now handles those deps for you like playwright. I have not tested this yet though, I'll give it a try today

Yeah in some of the later commits in this stack I update the readme for instructions on how to get this to work wrt to dependencies. We also have scripts in place: `yarn gentest` and `yarn gentest-validate` which abstracts that out that loader. As for chromedriver, according to https://www.selenium.dev/documentation/webdriver/troubleshooting/errors/driver_location/#use-the-latest-version-of-selenium, it appears that selenium now handles those deps for you like playwright. I have not tested this yet though, I'll give it a try today
joevilches (Migrated from github.com) reviewed 2023-12-13 12:10:42 -08:00
gentest/gentest-driver.ts
@@ -0,0 +40,4 @@
console.log('Generate', fileNameNoExtension);
// TODO: replace this with something more robust than just blindly replacing
// start/end in the entire fixture
joevilches (Migrated from github.com) commented 2023-12-13 12:10:42 -08:00
Copy Link

There are the other things like margin-start etc. I think we need a pretty wholistic system to replace things like that

There are the other things like `margin-start` etc. I think we need a pretty wholistic system to replace things like that
facebook-github-bot commented 2023-12-14 11:53:18 -08:00 (Migrated from github.com)
Copy Link

This pull request has been merged in facebook/yoga@98552078b1.

This pull request has been merged in facebook/yoga@98552078b1b93cafa04b2ee0060ad49df34510ac.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Merged
fb-exported
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1498
No description provided.
Delete Branch "export-D51874433"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?