-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests done #237
tests done #237
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## mrc-6023 #237 +/- ##
============================================
+ Coverage 97.67% 99.78% +2.10%
============================================
Files 189 188 -1
Lines 4563 4560 -3
Branches 1005 1006 +1
============================================
+ Hits 4457 4550 +93
+ Misses 98 9 -89
+ Partials 8 1 -7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good to me, just some comments on the README - could be addressed in a separate ticket.
config-static/README.md
Outdated
1. Gets the Javascript code for the ODE and discrete runners and saves them into `stores/runnerOde.js` and `stores/runnerDiscrete.js` (the runner are generic and shared across all stores) | ||
1. For each `<store-name>`, it saves the config into `stores/<store-name>/config.json` and it compiles the model code and saves the response into `stores/<store-name>/model.json`. | ||
|
||
We also build `wodin` frontend in static mode (just normal `wodin` frontend with an early return in the api service so we don't do any network requests) and copy the js and css files into the output folder. After this, we just commit the output folder into a github pages branch and github will deploy it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth saying here why we need to do early returns from the api service, and why it's benign that we do..?
Will the github pages bit be part of the main wodin build gha? It's implied here. We could make it an optional setting to do this - that would make things really easy for the user, but won't always be wanted if not using ghp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can add why we return early from the api service but i didnt want to write benign because i am not 100% it will always be benign, it is currently benign though...
so github pages just looks at a branch in your github repo with our normal setup, all we do in the action is push the html and js files to that branch. it is already optional because the user can just not hook up that branch to github pages, but this shall be explained in the README in the example repo!
return fs.mkdtempSync(tmpdir); | ||
}; | ||
|
||
export const tmpdirTest = test.extend<TmpDirTestFixture>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat! I've just been using memfs to fake the fs, but this is actually nicer because it cleans up after itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep its quite cool! want another excuse to use stuff like this more
expectPath(tmpdir); | ||
}); | ||
|
||
tmpdirTest("works as expected", async ({ tmpdir }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, classic test name 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yess, i shall try and improve it!
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks! Couple of tiny text suggestions.
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
Co-authored-by: Emma Russell <[email protected]>
havent change any functionality, just moved code around to different files to help make it easier to test!
we have talked through the functionality, the files we discussed in person were these ones so can just look at the tests in this PR and the comments I added, no need to focus on the javascript as much