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
4 changed files with 118 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
.eslintrc.cjs
View File

@@ -32,6 +32,7 @@ module.exports = {
},
env: {
es2020: true,
'shared-node-browser': true,
},
parserOptions: {
ecmaVersion: 'latest',

89
gentest/gentest-driver.ts Normal file
View File

@@ -0,0 +1,89 @@
/**
nicoburns commented 2023-12-13 06:15:34 -08:00 (Migrated from github.com)
Review
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.
joevilches commented 2023-12-13 12:09:33 -08:00 (Migrated from github.com)
Review
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
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict-local
* @format
*/
import * as fs from 'node:fs/promises';
import {format} from 'node:util';
import * as path from 'path';
import * as process from 'node:process';
import {Builder, logging} from 'selenium-webdriver';
import {Options} from 'selenium-webdriver/chrome.js';
await (async () => {
const options = new Options();
options.addArguments(
'--force-device-scale-factor=1',
'--window-position=0,0',
'--hide-scrollbars',
);
options.setLoggingPrefs({
browser: 'ALL',
performance: 'ALL',
});
const driver = await new Builder()
.forBrowser('chrome')
.setChromeOptions(options)
.build();
process.chdir(path.dirname(process.argv[1]));
const fixtures = await fs.readdir('./fixtures');
for (const fileName of fixtures) {
nicoburns commented 2023-12-13 06:04:24 -08:00 (Migrated from github.com)
Review
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).
joevilches commented 2023-12-13 12:05:01 -08:00 (Migrated from github.com)
Review
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.
const fixture = await fs.readFile('fixtures/' + fileName, 'utf8');
const fileNameNoExtension = path.parse(fileName).name;
console.log('Generate', fileNameNoExtension);
// TODO: replace this with something more robust than just blindly replacing
// start/end in the entire fixture
nicoburns commented 2023-12-13 06:37:09 -08:00 (Migrated from github.com)
Review
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.
joevilches commented 2023-12-13 12:10:42 -08:00 (Migrated from github.com)
Review
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
const ltrFixture = fixture
.replaceAll('start', 'left')
.replaceAll('end', 'right')
.replaceAll('flex-left', 'flex-start')
.replaceAll('flex-right', 'flex-end');
const rtlFixture = fixture
.replaceAll('start', 'right')
.replaceAll('end', 'left')
.replaceAll('flex-right', 'flex-start')
.replaceAll('flex-left', 'flex-end');
const template = await fs.readFile('test-template.html', 'utf8');
const f = await fs.open('test.html', 'w');
await f.write(
format(template, fileNameNoExtension, ltrFixture, rtlFixture, fixture),
);
await f.close();
await driver.get('file://' + process.cwd() + '/test.html');
const logs = await driver.manage().logs().get(logging.Type.BROWSER);
await fs.writeFile(
`../tests/generated/${fileNameNoExtension}.cpp`,
eval(logs[0].message.replace(/^[^"]*/, '')),
nicoburns commented 2023-12-13 06:20:50 -08:00 (Migrated from github.com)
Review
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?)
);
nicoburns commented 2023-12-13 06:06:41 -08:00 (Migrated from github.com)
Review
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 commented 2023-12-13 12:06:24 -08:00 (Migrated from github.com)
Review
Copy Link

ooo good callout. I can take a look at that

ooo good callout. I can take a look at that
await fs.writeFile(
`../java/tests/com/facebook/yoga/${fileNameNoExtension}.java`,
eval(logs[1].message.replace(/^[^"]*/, '')).replace(
'YogaTest',
fileNameNoExtension,
),
);
await fs.writeFile(
`../javascript/tests/generated/${fileNameNoExtension}.test.ts`,
eval(logs[2].message.replace(/^[^"]*/, '')).replace(
'YogaTest',
fileNameNoExtension,
),
);
}
await fs.unlink('test.html');
await driver.quit();
})();

14
gentest/package.json Normal file
View File

@@ -0,0 +1,14 @@
{
"name": "gentest",
"version": "0.0.0",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"type": "module",
"dependencies": {
"@types/node": "^20.10.3",
"@types/selenium-webdriver": "^4.1.21",
"babel-register-esm": "^1.2.5",
"selenium-webdriver": "^4.16.0"
}
}

14
gentest/tsconfig.json Normal file
View File

@@ -0,0 +1,14 @@
{
"compilerOptions": {
"target": "es2020",
"module": "nodenext",
"strict": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": false,
"moduleResolution": "nodenext",
"lib": [
"ES2021.String"
]
},
"files": ["gentest-driver.ts"]
}