search

flatironinstitute / mcmc-monitor

Monitor MCMC runs in the browser
Other
36 stars 0 forks source link

Include test as initial step in deploy script #48

Closed magland closed 1 year ago

magland commented 1 year ago

So perhaps in package.json, predeploy would be npm run build && npm run test

jsoules commented 1 year ago

This is now part of PR #45 as per this commit, which adds npm run coverage to the predeploy and (new) predev-deploy hooks.

I added an intentionally failing test (expect(1).toEqual(2)) and confirmed that a failed test will halt the deployment process under this setup, and that it's being triggered on both dev and prod deployment.

However, note the current setup has the tests living at the project level, so they aren't referenced in the (separate) package.json tooling for the service/src subtree (i.e. the daemon). That may take some further experimentation or configuration. I think ideally we want all the tests, for both client and server, to be part of the same organization, so I don't really want to reinstall vitest again in the subdirectory and have a separate testing infrastructure for that, but I don't know how achievable that will be.

magland commented 1 year ago

I think it makes sense to only test the GUI (not the service) when deploying the GUI. What do you think?

jsoules commented 1 year ago

Thus, suggesting we should have separate tests and testing setups, you mean?

I agree that makes sense, but it also tends to drive the projects further apart in a way. (I might need to ask you to remind me why the project is structured this way currently--vs having two projects with separate npm entries--I recall there was a reason, but I don't remember what it is.) It also just means more repeated configuration.

In practice, it should be a non-issue, because we wouldn't expect to deploy the GUI except from either the production/main branch or (for dev-deployment) from a feature branch that stems directly from the production/main branch; and we wouldn't expect to accept PRs into main (for either src/ or service/src) unless the tests all pass; so it shouldn't be possible to have failing service tests for a GUI update... but I appreciate the desire to avoid interference between them.

magland commented 1 year ago

Not sure whether existing directory structure is there for a reason. I think it could be two separate directories like this:

service/ gui/

But I'm not sure.

jsoules commented 1 year ago

Looks like it doesn't matter anyway, since (probably for very good reasons) I can't find a way to have the package.json in the child directory reference the scripting setup in the project root without doing something very hacky.

Regarding the directory structure--we essentially have two projects, the service (published on NPM) and the GUI (deployed on github-pages but not actually available on NPM for download). As they continue to develop separate infrastructure, it might make sense for them to just be separate repositories. During early rapid development it's easier not to have to push changes to a library in order to update the GUI, but I can't recall if there was an additional reason beyond that for having them share the same repo.

magland commented 1 year ago

My preference would be to keep them in one repo. I don't see the downside, and it keeps things in one place. Also, you should check whether some code (.ts files) are shared.

jsoules commented 1 year ago

Just added testing support for the service subpackage, along with adding testing as part of the command sequence for releasing. (Of course, we run that sequence of commands manually in the terminal, so it's really just making sure it's part of the cut-and-paste.)