Improvements to /learn/local

[islemaster]

Moves the JavaScript for the classroom map at /learn/local into apps, adds tests for some of it, and then refactors a whole lot of it. See the linked Jira item for motivation and specific concerns.

Recommendation for reviewers: Viewing individual commits may be helpful due to the amount of refactoring that happened here.

Links

• PLC-470

Testing story

I believe I've added unit tests to cover everything that I might have changed, but there is some moved code that I didn't add coverage for, so please don't assume it's comprehensive.

Reviewer Checklist:

• Tests provide adequate coverage
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[islemaster]

Quick check on this first commit please, @davidsbailey: Did I move this pegasus JS into apps correctly, using the new webpack helper?

[davidsbailey]

@islemaster yes, looks good!

[hacodeorg]

Great improvement! We also tested it against bugcrowd example and it worked!

[breville]

Wow, that's a big change, but going through the commits one by one was a good suggestion, and everything looks to be improved. Thank you!

[islemaster]

Well, I tried changing our coverage reporter to work around the Drone issue, but my build is still failing - now with even less helpful logging.

2862 | 31 10 2019 21:42:25.176:INFO [karma]: Karma v1.7.0 server started at http://0.0.0.0:9876/ | 748s
2863 | 31 10 2019 21:42:25.177:INFO [launcher]: Launching browser PhantomJS with unlimited concurrency | 748s
2864 | 31 10 2019 21:42:25.491:INFO [launcher]: Starting browser PhantomJS | 748s
2865 | 31 10 2019 21:42:26.151:ERROR [phantomjs.launcher]: Fontconfig warning: ignoring C.UTF-8: not a valid language tag | 748s
2866 |   | 748s
2867 | 31 10 2019 21:42:27.727:INFO [PhantomJS 2.1.1 (Linux 0.0.0)]: Connected on socket JzvTtJC0GH9F3wk7AAAA with id 37819356 | 748s
2868 | PhantomJS 2.1.1 (Linux 0.0.0) LOG: Object{study: 'assignment', study_group: 'v0', event: 'create_section', data_json: '{"section_id":12,"section_creation_timestamp":"2019-10-21T23:45:34.345Z","script_id":35,"course_id":null}', created_at: '2019-10-31T21:42:36.181Z', environment: 'development', uuid: 'b5f060ac-75e8-44d6-af58-77d92c40d781', device: '{"user_agent":"Mozilla/5.0 (Unknown; Linux x86_64) AppleWebKit/538.1 (KHTML, like Gecko) PhantomJS/2.1.1 Safari/538.1","window_width":400,"window_height":300,"hostname":"localhost-studio.code.org","full_path":"http://localhost-studio.code.org:9876/context.html"}', script_id: null, level_id: NaN} | 748s
2869 | PhantomJS 2.1.1 (Linux 0.0.0) LOG: Object{study: 'assignment', study_group: 'v0', event: 'create_section', data_json: '{"section_id":12,"section_creation_timestamp":"2019-10-21T23:45:34.345Z","script_id":35,"course_id":null}', created_at: '2019-10-31T21:42:36.181Z', environment: 'development', uuid: 'b5f060ac-75e8-44d6-af58-77d92c40d781', device: '{"user_agent":"Mozilla/5.0 (Unknown; Linux x86_64) AppleWebKit/538.1 (KHTML, like Gecko) PhantomJS/2.1.1 Safari/538.1","window_width":400,"window_height":300,"hostname":"localhost-studio.code.org","full_path":"http://localhost-studio.code.org:9876/context.html"}', script_id: null, level_id: NaN} | 748s
2870 | PhantomJS 2.1.1 (Linux 0.0.0) LOG: Object{study: 'assignment', study_group: 'v0', event: 'create_section', data_json: '{"section_id":12,"section_creation_timestamp":"2019-10-21T23:45:34.345Z","script_id":35,"course_id":null}', created_at: '2019-10-31T21:42:36.181Z', environment: 'development', uuid: 'b5f060ac-75e8-44d6-af58-77d92c40d781', device: '{"user_agent":"Mozilla/5.0 (Unknown; Linux x86_64) AppleWebKit/538.1 (KHTML, like Gecko) PhantomJS/2.1.1 Safari/538.1","window_width":400,"window_height":300,"hostname":"localhost-studio.code.org","full_path":"http://localhost-studio.code.org:9876/context.html"}', script_id: null, level_id: NaN} | 748s
2871 | PhantomJS 2.1.1 (Linux 0.0.0) LOG: Object{study: 'assignment', study_group: 'v0', event: 'create_section', data_json: '{"section_id":12,"section_creation_timestamp":"2019-10-21T23:45:34.345Z","script_id":null,"course_id":9999}', created_at: '2019-10-31T21:42:36.183Z', environment: 'development', uuid: 'b5f060ac-75e8-44d6-af58-77d92c40d781', device: '{"user_agent":"Mozilla/5.0 (Unknown; Linux x86_64) AppleWebKit/538.1 (KHTML, like Gecko) PhantomJS/2.1.1 Safari/538.1","window_width":400,"window_height":300,"hostname":"localhost-studio.code.org","full_path":"http://localhost-studio.code.org:9876/context.html"}', script_id: null, level_id: NaN} | 748s
2872 | PhantomJS 2.1.1 (Linux 0.0.0) LOG: Object{study: 'assignment', study_group: 'v0', event: 'create_section', data_json: '{"section_id":12,"section_creation_timestamp":"2019-10-21T23:45:34.345Z","script_id":null,"course_id":9999}', created_at: '2019-10-31T21:42:36.183Z', environment: 'development', uuid: 'b5f060ac-75e8-44d6-af58-77d92c40d781', device: '{"user_agent":"Mozilla/5.0 (Unknown; Linux x86_64) AppleWebKit/538.1 (KHTML, like Gecko) PhantomJS/2.1.1 Safari/538.1","window_width":400,"window_height":300,"hostname":"localhost-studio.code.org","full_path":"http://localhost-studio.code.org:9876/context.html"}', script_id: null, level_id: NaN} | 748s
2873 | PhantomJS 2.1.1 (Linux 0.0.0) LOG: Object{study: 'assignment', study_group: 'v0', event: 'create_section', data_json: '{"section_id":12,"section_creation_timestamp":"2019-10-21T23:45:34.345Z","script_id":null,"course_id":9999}', created_at: '2019-10-31T21:42:36.183Z', environment: 'development', uuid: 'b5f060ac-75e8-44d6-af58-77d92c40d781', device: '{"user_agent":"Mozilla/5.0 (Unknown; Linux x86_64) AppleWebKit/538.1 (KHTML, like Gecko) PhantomJS/2.1.1 Safari/538.1","window_width":400,"window_height":300,"hostname":"localhost-studio.code.org","full_path":"http://localhost-studio.code.org:9876/context.html"}', script_id: null, level_id: NaN} | 748s
2874 | LOG: '%cDid you just try to use p5.js's arc() function? If so, you may want to move it into your sketch's setup() function. | 748s
2875 |   | 748s
2876 | For more details, see: https://github.com/processing/p5.js/wiki/Frequently-Asked-Questions#why-cant-i-assign-variables-using-p5-functions-and-variables-before-setup', 'color: #B40033' | 748s
2877 | LOG: '%cDid you just try to use p5.js's arc() function? If so, you may want to move it into your sketch's setup() function. | 748s
2878 |   | 748s
2879 | For more details, see: https://github.com/processing/p5.js/wiki/Frequently-Asked-Questions#why-cant-i-assign-variables-using-p5-functions-and-variables-before-setup', 'color: #B40033' | 748s
2880 | LOG: '%cDid you just try to use p5.js's arc() function? If so, you may want to move it into your sketch's setup() function. | 748s
2881 |   | 748s
2882 | For more details, see: https://github.com/processing/p5.js/wiki/Frequently-Asked-Questions#why-cant-i-assign-variables-using-p5-functions-and-variables-before-setup', 'color: #B40033' | 748s
2883 | LOG: '%cDid you just try to use p5.js's arc() function? If so, you may want to move it into your sketch's setup() function. | 748s
2884 |   | 748s
2885 | For more details, see: https://github.com/processing/p5.js/wiki/Frequently-Asked-Questions#why-cant-i-assign-variables-using-p5-functions-and-variables-before-setup', 'color: #B40033' | 748s
2886 | LOG: '%cDid you just try to use p5.js's arc() function? If so, you may want to move it into your sketch's setup() function. | 748s
2887 |   | 748s
2888 | For more details, see: https://github.com/processing/p5.js/wiki/Frequently-Asked-Questions#why-cant-i-assign-variables-using-p5-functions-and-variables-before-setup', 'color: #B40033' | 748s
2889 | LOG: '%cDid you just try to use p5.js's arc() function? If so, you may want to move it into your sketch's setup() function. | 748s
2890 |   | 748s
2891 | For more details, see: https://github.com/processing/p5.js/wiki/Frequently-Asked-Questions#why-cant-i-assign-variables-using-p5-functions-and-variables-before-setup', 'color: #B40033' | 748s
2892 |   | 748s
2893 | Finished in 1.025 secs / 0 secs @ 21:42:41 GMT+0000 (UTC) | 748s
2894 |   | 748s
2895 | SUMMARY: | 748s
2896 | ✔ 0 tests completed | 748s
2897 | 31 10 2019 21:42:46.094:WARN [launcher]: PhantomJS was not killed in 2000 ms, sending SIGKILL. | 748s
2898 | Warning: Task "karma:unit" failed.� Use --force to continue. | 748s
2899 |   | 748s
2900 | Aborted due to warnings. | 748s
2901 | 2 : 1572558019.620 146.673 0 14182 3 0 (PORT=9876 node --max_old_space_size=4096 /drone/src/apps/node_modules/.bin/grunt unitTest && /tmp/codecov.sh -C 9183adc3000068f838cd01a5f5c8588f90333b05 -cF unit) | 748s
2902 | parallel: This job failed: | 748s
2903 | (PORT=9876 node --max_old_space_size=4096 /drone/src/apps/node_modules/.bin/grunt unitTest && /tmp/codecov.sh -C 9183adc3000068f838cd01a5f5c8588f90333b05 -cF unit) | 748s
2904 | npm ERR! code ELIFECYCLE | 748s
2905 | npm ERR! errno 3 | 748s
2906 | npm ERR! blockly-mooc@0.0.161 test-low-memory: `./test-low-memory.sh` | 748s
2907 | npm ERR! Exit status 3 | 748s
2908 | npm ERR! | 748s
2909 | npm ERR! Failed at the blockly-mooc@0.0.161 test-low-memory script. | 748s
2910 | npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

I'm not sure where the firehose logging or p5.js arc() warnings are coming from, but I have no reason to believe they'd be connected to my new test. I'll have to keep digging I guess - maybe the istanbul issue was a red herring, or a legitimate problem but only one of multiple flaky issues this PR is running into?

[islemaster]

Comparing my failure log to a successful run:

• Fontconfig warning: ignoring C.UTF-8: not a valid language tag shows up in both logs
• study: 'assignment' only showed up in the failed logs, but lots of other firehose logging appears in the successful log.
• The p5.js arc() function error only shows up in the failed log.

[davidsbailey]

sorry to hear it, @islemaster . it sure seems unclear whether the istanbul fix has let you get farther along or not. a few ideas that might be faster than a full binary search:

• temporarily disable the new unit test in this PR
• temporarily disable p5-related unit tests

[islemaster]

Skipping all the tests in localTest.js still fails. (Drone)
Removing localTest.js entirely passes. (Drone)
Skipping the tests and deleting the fixtures at the end of the test file still fails. (Drone)
Removing almost all of the test file fails. (Drone)
Removing all meaningful content from the test, including the actual import of the source I'm trying to test passes. (Drone - UI tests failed, but unit tests passed)

Current hypothesis: Importing the file being tested is what's breaking the build.

Put back the import and remove parts of local.js passes. (Drone)
Removing only the onload code from local.js passes. (Drone)

[islemaster]

Okay, PTAL. I believe I've tracked down all the test issues that were causing this PR to fail on Drone:

• The initial bug in the karma-coverage seemed legitimate, and was resolved by switching to karma-coverage-istanbul-reporter here. (Which seems like it was a good idea anyway.)
• That uncovered another failure with an infuriating lack of helpful output. It turns out the file I was trying to test included some onload code that was causing a problem, so I extracted the particular code I wanted to test to yet another file to be tested in isolation.
• Then PhantomJS (correctly) complained about me setting a style attribute directly, a thing we shouldn't do but Chrome allows it. From MDN:

  Styles should not be set by assigning a string directly to the style property (as in elt.style = "color: blue;"), since it is considered read-only, as the style attribute returns a CSSStyleDeclaration object which is also read-only.

• Then PhantomJS failed because I tried to call forEach on a NodeList, which is a thing you should be able to do but some very old browsers don't support it - including PhantomJS, apparently.

I can't wait until we move our tests to headless chrome.

[davidsbailey]

Great work, Brad! It seems like we can definitely chalk these up as developer days lost to not having upgraded yet to headless Chrome.