fix(build): build/test on windows by yamadayutaka · Pull Request #6431 · RaspberryPiFoundation/blockly

yamadayutaka · 2022-09-15T14:07:22Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide
I ran npm run format and npm run lint

The details

Resolves

Proposed Changes

chore(deps): bump @hyperjump/json-schema from 0.18.4 to 0.18.5
chore(deps): add gulp-gzip 1.4.2
build: migrate test scripts to gulp task (test_tasks.js)
build: not to use the grep command
build: normalize path

Behavior Before Change

The following commands are failing.

npm run build
npm run test

Behavior After Change

The following commands are successful.

npm run build
npm run test

Reason for Changes

For development on Windows environment.

Test Coverage

Confirmed in the following environments.

Windows10 / Node.js v16.17.0 / npm 8.15.0
Windows10 / Node.js v14.18.0 / npm 8.19.2
Ubuntu20.04 / Node.js v16.17.0 / npm 8.15.0
GitHub Actions (ubuntu-latest/windows-latest 14.x/16.x)
https://github.com/yamadayutaka/blockly/actions/runs/3060923144

* chore(deps): bump @hyperjump/json-schema from 0.18.4 to 0.18.5 * chore(deps): add gulp-gzip 1.4.2 * build: migrate test scripts to gulp task (test_tasks.js) * build: not to use the grep command * build: normalize path

cpcallen

Hello Yutaka,

Thank you for sending us this substantial and very important PR. We definitely want to make it easier for Windows-using developers to be able to contribute to Blockly, and we know that their experience at the moment is not great. We really appreciate your efforts to help improve it.

Overall this PR looks very good, but there are some changes that will be needed before we can merge it. Most are minor (see below), but there are few slightly higher-level ones too:

Our current run_all-_tests script will plow ahead even when some of the tests fail. That's actually not great, because if build-debug fails we don't want to run metadata, mocha, generators, package, or node, but on the other hand your changes mean that an eslint failure will prevent any subsequent tests from running, which is too harsh. We want to run as many tests as we sensible can even if some of them fail—while making sure that GitHub CI still knows that there were failures.
It would be better to maintain the same nice formatting of test output as previously. See detailed comment below about this, but note also that it would be nice if we could suppress the "Starting" and "Finished" messages generated by Gulp.
There are quite a few functions in test_tasks.js that could probably be written more concisely as async functions.

Finally:

There is also the question of whether we want to be converting shell scripts to Gulp tasks. I put this question to the team, and the main observations were:

there is a general preference for node scripts over bash scripts, but
we're probably going to try to remove Gulp from our build system before too long.

To that end: your test_tasks.js is a good step on the road from bash to plain JS, and I'm happy to accept it with only minor fixes as noted—but please feel free also to modify it to NOT to use gulp features (and async features generally) where it doesn't need to: making it more like a plain, synchronous node script like tests/migration/validate-renamings.js could make it simpler and will probably be more helpful to us in the long run.

cpcallen · 2022-09-29T15:01:46Z

+  if (failed === 0) {
+    done();
+  }


The callback should always be (eventually) called, but it can be passed an Error if the task fails. Try something like:

Suggested change

if (failed === 0) {

done();

}

done(failed > 0 ? new Error(/* useful message */) : undefined);

I changed the implementation to no callbacks.

cpcallen · 2022-09-29T15:04:20Z

+function generators(done) {
+  fs.mkdirSync(TMP_DIR);
+
+  execSync('node tests/generators/run_generators_in_browser.js');


Could this just be a require('../../tests/generators/run_generators_in_browser.js)`?

I changed as suggested.

I changed the execution of run_mocha_tests_in_browser.js as well.

cpcallen · 2022-09-29T15:06:09Z

+  if (failed === 0) {
+    console.log(`${BOLD_GREEN}All generator tests passed.${ANSI_RESET}`);
+    done();
+  } else {
+    console.log(`${BOLD_RED}Failures in ${failed} generator tests.${ANSI_RESET}`);
+  }


As above: done should always be called.

I changed the implementation to no callbacks.

yamadayutaka · 2022-09-30T00:50:50Z

Thank you for reviewing my PR.
I will check and fix what you pointed out.

yamadayutaka · 2022-10-03T08:16:18Z

Hello @cpcallen ,

I changed it based on your comments, so please check it.
Also, I made the following changes:

Formatting test output as previously.
Always continue even if a test unit fails.
Suppress the gulp messages.
Fix test_tasks.js to pass eslint.

* Add JSDoc comment * Line length <= 80 characters. * Formatting test output as previously. * Always continue even if a test unit fails. * Suppress the gulp messages. * Fix test_tasks.js to pass eslint.

yamadayutaka · 2022-10-07T03:18:29Z

Sorry, I overlooked "hidden conversations".
I will check and fix what you pointed out.

* Change generator test output directory. * Formatting test output as previously.

yamadayutaka · 2022-10-07T13:21:45Z

I also fixed the overlooked content, so please check it.

cpcallen

Sorry it's taken me so long to re-review this PR. With your updates everything is looking great. Thanks for taking the time to do this work, and then additionally to respond so positively to all of my nit-picking. A big thank-you from all of us.

cpcallen · 2022-10-14T18:08:39Z

+    entry: posixPath((argv.compileTs) ?
+      path.join(TSC_OUTPUT_DIR, CORE_DIR, 'main.js') :
+      path.join(CORE_DIR, 'main.js')),


It appears that you are trying to helpfully merge in code from getChunkOptions that I should have deleted that code in PR #6220. Ooops! Since it obviously hasn't been causing any problems until now I'm not going to block this PR because of my own mistake.

I believe that if I had removed that code as I intended to then the you would have written this instead, and I will apply this fix in another PR I am preparing that touches all of this again anyway:

Suggested change

entry: posixPath((argv.compileTs) ?

path.join(TSC_OUTPUT_DIR, CORE_DIR, 'main.js') :

path.join(CORE_DIR, 'main.js')),

entry: posixPath(path.join(CORE_DIR, 'main.js')),

cpcallen · 2022-10-27T15:26:39Z

-  if (argv.compileTs) {
-    chunks[0].entry = path.join(TSC_OUTPUT_DIR, chunks[0].entry);
-  }


Yeah, I should have deleted this bit a long time ago.

cpcallen · 2022-10-28T22:34:17Z

Hi @yamadayutaka: @rachel-fenichel ran in to a small problem with npm run build:generators on Linux, which turned out to be caused by the trailing / in the call rimraf('build/generators/'). She's fixed this in PR #6588; can you check to make sure that that change does not break anything on Windows?

yamadayutaka · 2022-10-29T05:00:09Z

Hi @cpcallen and @rachel-fenichel, Sorry for the lack of confirmation. Thank you for your fix.
I have confirmed that it works fine on Windows.

cpcallen · 2022-11-01T14:32:45Z

Hi @cpcallen and @rachel-fenichel, Sorry for the lack of confirmation. Thank you for your fix. I have confirmed that it works fine on Windows.

Thanks for the confirmation!

The bash script tests/run_all_tests.js was deleted in PR RaspberryPiFoundation#6431, where it was replaced by `scripts/gulpfiles/test_tasks.js`, but it was inadvertently resurrected by PR RaspberryPiFoundation#6475, apparently due to an error when manually resolving conflicts for merge commit 00676ed. Begone, undead script!

The bash script tests/run_all_tests.sh was deleted in PR #6431, where it was replaced by scripts/gulpfiles/test_tasks.js, but it was inadvertently resurrected by PR #6475, apparently due to an error when manually resolving conflicts for merge commit 00676ed. Begone, undead script!

yamadayutaka requested a review from a team as a code owner September 15, 2022 14:07

yamadayutaka requested a review from NeilFraser September 15, 2022 14:07

github-actions Bot assigned NeilFraser Sep 15, 2022

build: build/test on windows

a4cf041

* chore(deps): bump @hyperjump/json-schema from 0.18.4 to 0.18.5 * chore(deps): add gulp-gzip 1.4.2 * build: migrate test scripts to gulp task (test_tasks.js) * build: not to use the grep command * build: normalize path

yamadayutaka force-pushed the feature/build_test_windows branch from e9aeb12 to a4cf041 Compare September 15, 2022 14:40

NeilFraser requested review from cpcallen and removed request for NeilFraser September 28, 2022 17:09

github-actions Bot assigned cpcallen Sep 28, 2022

NeilFraser removed their assignment Sep 28, 2022

cpcallen requested changes Sep 29, 2022

View reviewed changes

yamadayutaka force-pushed the feature/build_test_windows branch from de7a728 to 0090d4b Compare October 3, 2022 08:14

fix: Modified based on review suggestions.

0b42d80

* Add JSDoc comment * Line length <= 80 characters. * Formatting test output as previously. * Always continue even if a test unit fails. * Suppress the gulp messages. * Fix test_tasks.js to pass eslint.

yamadayutaka force-pushed the feature/build_test_windows branch from 0090d4b to 0b42d80 Compare October 3, 2022 11:20

yamadayutaka added 3 commits October 7, 2022 17:33

fix: Modified based on review suggestions.

4405364

* Change generator test output directory. * Formatting test output as previously.

fix: Formatting test output as previously.

0631e06

fix: Modified based on review suggestions.

93e95e0

cpcallen mentioned this pull request Oct 17, 2022

feat(build): Support TypeScript in core/ #6220

Merged

4 tasks

cpcallen approved these changes Oct 27, 2022

View reviewed changes

Merge branch 'develop' into feature/build_test_windows

3069e29

cpcallen merged commit 52879dd into RaspberryPiFoundation:develop Oct 27, 2022

cpcallen changed the title ~~build: build/test on windows~~ fix(build): build/test on windows Oct 27, 2022

github-actions Bot added the PR: fix Fixes a bug label Oct 27, 2022

rachel-fenichel mentioned this pull request Oct 28, 2022

fix(tests): generator tests on linux #6588

Merged

4 tasks

cpcallen mentioned this pull request Oct 31, 2022

chore(build): Remove build products from the Blockly repository #6475

Merged

4 tasks

cpcallen mentioned this pull request Nov 17, 2022

chore(build): Re-delete tests/run_all_tests.sh #6636

Merged

4 tasks

maribethb mentioned this pull request Jan 11, 2023

Build failing on Windows #5967

Closed

cpcallen linked an issue Jan 11, 2023 that may be closed by this pull request

Build failing on Windows #5967

Closed

cpcallen mentioned this pull request Jan 13, 2023

Generator tests are flaky #6757

Closed

Conversation

yamadayutaka commented Sep 15, 2022

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Uh oh!

cpcallen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yamadayutaka commented Sep 30, 2022

Uh oh!

yamadayutaka commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamadayutaka commented Oct 7, 2022

Uh oh!

yamadayutaka commented Oct 7, 2022

Uh oh!

cpcallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpcallen commented Oct 28, 2022

Uh oh!

yamadayutaka commented Oct 29, 2022

Uh oh!

cpcallen commented Nov 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yamadayutaka commented Oct 3, 2022 •

edited

Loading