Cardinal Directions for Roofs and Misc Roof Fixes #61
Conversation
…ofs; removing warning of direction=0 for skillion roofs
|
Could you add a test for the new cardinal direction code? And one that demonstrates that a building with a north roof does not produce an entry in the error log, but one with a missing direction does? |
| @@ -132,7 +132,7 @@ function createFolders(folder, options) { | |||
| if (property === 'colour') { | |||
| roofFolder.addColor(options.roof, property); | |||
| } else if (property === 'shape') { | |||
| const roofTypesAvailable = ['dome', 'flat', 'gabled', 'onion', 'pyramidal', 'skillion', 'hipped', 'round', 'gambrel', 'round']; | |||
| const roofTypesAvailable = ['dome', 'flat', 'gabled', 'onion', 'pyramidal', 'skillion', 'hipped', 'round', 'gambrel']; | |||
| // If this roof is not supported, add it to the list for sanity. | |||
| if (!roofTypesAvailable.includes(options.roof.shape)) { | |||
| roofTypesAvailable.push(options.roof.shape); | |||
There was a problem hiding this comment.
(pinned to a line of code to allow for threaded discussion)
Beakerboy commented Jul 8, 2024, 7:02 AM PDT
Could you add a test for the new cardinal direction code? And one that demonstrates that a building with a north roof does not produce an entry in the error log, but one with a missing direction does?
I don't see instructions on your up-and-coming contribution guide for running tests. Is there a simple method?
There was a problem hiding this comment.
Yeah, it’s in the GitHub workflow. They should automatically run when you check in your code.
CI=true NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules" npm test
Isn’t this the standard way to run tests?
There was a problem hiding this comment.
Also. On Wisconsin! I grew up in Wausau and got my Chemical Engineering degree there. Class of 2000.
There was a problem hiding this comment.
There seems to be some pre-steps needed to locally run the unit test:
% CI=true NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules" npm test
> [email protected] test
> c8 jest
(node:66111) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66114) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66113) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66112) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66111) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
PASS test/utils.test.js
FAIL test/split_way_multipolygon.test.js
● Test suite failed to run
Cannot find module 'pyramid' from 'src/buildingpart.js'
at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
(node:66113) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
FAIL test/multipolygon.test.js
● Test suite failed to run
Cannot find module 'pyramid' from 'src/buildingpart.js'
at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
(node:66114) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
FAIL test/building.test.js
● Test suite failed to run
Cannot find module 'pyramid' from 'src/buildingpart.js'
at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
(node:66112) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
Test Suites: 3 failed, 1 passed, 4 total
Tests: 9 passed, 9 total
Snapshots: 0 total
Time: 1.546 s
Ran all test suites.
=============================== Coverage summary ===============================
Statements : 16.56% ( 265/1600 )
Branches : 66.66% ( 18/27 )
Functions : 33.33% ( 7/21 )
Lines : 16.56% ( 265/1600 )
================================================================================Generally one runs unit tests locally before the CI/CD runs them. Otherwise it can be a long dev test-fail-iterate cycle.
There was a problem hiding this comment.
I’m not a real programmer and usually just code on my phone using GitHub as my IDE. I can’t run SSH at work, and when I get home I don’t use my computer, so this is it. The GitHub workflow has the wget or curl statement I use to pull down those other files.
|
I submitted a pull request to your branch. Please review it and let me know what you think. I refactored your function to make is shorter and created a unit test for it. It still needs a test for the message in the error log. |
| @@ -132,7 +132,7 @@ function createFolders(folder, options) { | |||
| if (property === 'colour') { | |||
| roofFolder.addColor(options.roof, property); | |||
| } else if (property === 'shape') { | |||
| const roofTypesAvailable = ['dome', 'flat', 'gabled', 'onion', 'pyramidal', 'skillion', 'hipped', 'round', 'gambrel', 'round']; | |||
| const roofTypesAvailable = ['dome', 'flat', 'gabled', 'onion', 'pyramidal', 'skillion', 'hipped', 'round', 'gambrel']; | |||
| // If this roof is not supported, add it to the list for sanity. | |||
| if (!roofTypesAvailable.includes(options.roof.shape)) { | |||
| roofTypesAvailable.push(options.roof.shape); | |||
There was a problem hiding this comment.
(tagged on a line of code to allow for threaded discussion)
Beakerboy commented Jul 18, 2024, 4:28 AM PDT
I submitted a pull request to your branch. Please review it and let me know what you think. I refactored your function to make is shorter and created a unit test for it. It still needs a test for the message in the error log.
I added some feedback to your PR (on my PR) -- justinormont#1
Additions:
Fixes:
roof.direction='0'(north-facing) for skillion roofsCloses #60