search

natcap / urban-online-workflow

This repository hosts the beta implementation of the Urban Online ES Workflow. The project is intended to give urban planners the ability to create and assess scenarios using InVEST Urban models.
1 stars 5 forks source link

A test suite for the frontend & a GHA workflow #38

Closed davemfish closed 1 year ago

davemfish commented 1 year ago

This PR adds vitest for running frontend tests. So far there is just one test, but coverage shows that it at least imports all of the source code, and that was the main goal for now.

@emlys you'll see in the new workflow file that a .env is created with mapbox access tokens, since that's where vite and our app expect to find those tokens. I added those secrets in my own fork, but did not add them to natcap's fork because secrets aren't available during PRs anyway. So the tests fail for that reason here. But are passing on my fork: https://github.com/davemfish/urban-online-workflow/actions/runs/2720422093

Probably we need a new strategy going forward. Here's one idea: Move the tokens out of the .env file and onto the backend, somewhere where the app can request the tokens when the page first loads, and then use them to make the subsequent requests to mapbox. If we want to do this, I can make another issue for it and we can discuss the backend option with James & Doug. Until then, maybe it's better than nothing to have passing checks on our forks, but not on upstream?

I feel like there should be a "correct" way to manage this stuff, as people must be doing it all the time. Maybe I just don't know the exact right language for searching for the answer.

davemfish commented 1 year ago

this is awesome @davemfish! It ran smoothly for me locally. I did get a couple warnings but I don't think it's related to the test setup.

(node:53659) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
stderr | unknown test
Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
    at App (/Users/emily/urban-online-workflow/frontend/src/App.jsx:13:53)

Right, I see this too. Maybe we can be sure to address this during #27 , as I imagine we'll be touching a lot of those useEffect hooks there. And if we update to React 18 in the process, that might force us to be even more strict about async useEffect (see my note on #27 )

Looks like it's failing on GHA with an URL error though, not sure what's up with that.

I believe that's the missing mapbox tokens right there

davemfish commented 1 year ago

Thanks, @emlys , back to you! Latest Actions on my fork are passing: https://github.com/davemfish/urban-online-workflow/actions

emlys commented 1 year ago

I believe that's the missing mapbox tokens right there

Oh right! It's so easy to forget about