Make run button functional #25

david-mears-2 · 2024-09-06T00:08:00Z

Add a functional submit button and all the related gubbins: api endpoint and handler, tests.
Use a pinia store for largeScreen

You can test this locally by seeing if clicking the run button takes you to a page showing you the dummy run id.

This was necessitated because the nuxt test utils don't seem to have a way to pass through a body.

codecov · 2024-09-06T00:09:45Z

Codecov Report

Attention: Patch coverage is 85.45455% with 16 lines in your changes missing coverage. Please review.

Project coverage is 61.02%. Comparing base (f430419) to head (3e43583).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
server/api/scenarios.post.ts	0.00%	9 Missing and 1 partial ⚠️
layouts/default.vue	81.25%	2 Missing and 1 partial ⚠️
components/ParameterForm.vue	93.54%	2 Missing ⚠️
types/storeTypes.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   58.70%   61.02%   +2.32%     
==========================================
  Files          26       32       +6     
  Lines         724      798      +74     
  Branches       63       74      +11     
==========================================
+ Hits          425      487      +62     
- Misses        290      300      +10     
- Partials        9       11       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

absternator

Nice work !!! looks clean and works great!!!!!! just left some minor comments

absternator · 2024-09-06T11:13:38Z

components/ParameterForm.vue

+  formSubmitting.value = true;
+  const response = await $fetch<NewScenarioData>("/api/scenarios", {
+    method: "POST",
+    query: { // Using query instead of body because I couldn't work out how to send a body in the integration test.


should defs use body here... your code shouldn't change to accommodate the tests...

I don't have a reason to prefer body over query; is there one?

Semantically the use of query params is incorrect here... they are more for filtering, sorting / picking certain ids... when doing post request and sending data its semantically correct to use a body

Agreed - structured data should be sent in a POST body not in a single qs param, that's our standard approach for submitting a run. It's easier to validate, more readable etc.
Must be some way of getting the integration test to accept a body - if not, we might need a different integration test approach!

I tried for some time to use the nuxt test utils to pass through a body, but there didn't seem to be a way.
To be clear, the R API is still receiving a body, it's only the web app that's taking a query parameter.

I tried again today with a fresh brain and am making headway :)

absternator · 2024-09-06T11:17:26Z

components/ParameterForm.vue

 });
+
+const appStore = useAppStore();


should move this at top with all variable definitions... nice to keep nicely separated with methods

components/ParameterForm.vue

absternator · 2024-09-06T11:20:28Z

components/ParameterForm.vue

+
+  if (response) {
+    const { runId } = response;
+    navigateToData.value = `/scenarios/${runId}`;


Suggested change

navigateToData.value = `/scenarios/${runId}`;

await navigateTo(`/scenarios/${runId}`)

unless reason for assignment before?

This is for testing only, because I couldn't get a spy on navigateTo.

Again, this seems like an issue with the testing approach that isn't ideally solved by putting unnecessary stuff in the code. And if we really really need the crud it ought to be commented as such.

I had the comment in the test file - there are two other places it could be, here, and by the data attribute itself.

I think our only option besides this is just not to test the form submission as part of component tests. That means we would rely only on e2e tests for the form submission. But since our e2e test doesn't mock the R API, we wouldn't be able to verify that the correct parameters were passed to it by the component.

Well, until such time as it can test the result page, I suppose.

Emma found this solution: #29

components/SideBar.vue

absternator · 2024-09-06T11:26:56Z

server/api/scenarios.post.ts

+
+export default defineRApiEventHandler(
+  async (event): Promise<NewScenarioResponse> => {
+    const query = getQuery(event);


as per prev comment should be reading body not query params

absternator · 2024-09-06T11:27:15Z

server/api/scenarios.post.ts

+
+    const modelParameters = JSON.parse(query.parameters as string) as ParameterDict;
+
+    // Delegate to runScenario so that the logic can be unit-tested.


dont need this comment

I've moved this comment - which was getting duplicated for each api endpoint - into a new .md "API design decisions.md"

absternator · 2024-09-06T11:28:28Z

server/handlers/scenarios.ts

+
+const rApiNewScenarioEndpoint = "/scenario/run";
+
+// Send a request to the R API for a job run.


dont neeed comments again.. the code you wrote is clean and understandable

absternator · 2024-09-06T11:30:11Z

stores/appStore.ts

+    initializeAppState(): void {
+      this.largeScreen = true;
+    },
+    setScreenSize(bool: boolean): void {


should this be setScreenSizeToLarge .. also function param should be descrptive .. eg. isLargeScreen rather than bool

Good points.

I have changed the function param to be called large, otherwise kept this the same

EmmaLRussell

Great stuff, good to get the store in.

Seems like some of the testing was a headache, which I'm probably adding to with some of these comments.. I think it would be really good to avoid some of these test-related-code-polluting additions if possible. Have made a couple of suggestions which you may have already tried. Happy to have a look at any of these if you're sick of bashing against them yourself - can't guarantee to get anywhere, as this is mostly all new to me too, but I think it's worth solving these now if we can, and can avoid proliferation.

EmmaLRussell · 2024-09-10T15:10:56Z

components/ParameterForm.vue

@@ -1,9 +1,11 @@
 <template>
-  <div>
+  <div v-show="pageMounted">


Why is this actually necessary? A Playwright timing issue? It should be possible to write Playwright tests so that they wait for a condition to become true. I think I'd rather have messy tests than somewhat inexplicable properties of the code that are only there because of test behaviour. If it really isn't then let's have a comment explaining it.

Luckily, this problem disappears next PR, because by then we can wait for version data as our signal that everything is mounted.

EmmaLRussell · 2024-09-10T15:14:39Z

components/ParameterForm.vue

    <CForm
      v-if="props.metadata && formData"
      class="inputs"
-      :data-test="JSON.stringify(formData)"
+      role="form"
+      :data-test-form-data="JSON.stringify(formData)"


Again, this feels like it should be avoidable - if it's for component testing, can't we just examine the values in the rendered elements?

Removed the dependency on this from tests. In the component test, I think the test relying on this was redundant anyway.

EmmaLRussell · 2024-09-10T15:16:39Z

components/ParameterForm.vue

@@ -36,6 +38,7 @@
                autocomplete="off"
                :label="option.label"
                :value="option.id"
+                :disabled="!pageMounted"


We shouldn't be shown if page isn't mounted, so not sure this is needed.

Removed in next PR.

EmmaLRussell · 2024-09-10T15:16:51Z

components/ParameterForm.vue

@@ -49,7 +52,8 @@
            :id="parameter.id"
            v-model="formData[parameter.id]"
            :aria-label="parameter.label"
-            class="form-select" :class="[largeScreen ? 'form-select-lg' : '']"
+            class="form-select" :class="[screenIsLarge ? 'form-select-lg' : '']"
+            :disabled="!pageMounted"


Removed in next PR.

EmmaLRussell · 2024-09-10T15:23:02Z

components/ParameterForm.vue

+
+  if (response) {
+    const { runId } = response;
+    navigateToData.value = `/scenarios/${runId}`;


Again, this seems like an issue with the testing approach that isn't ideally solved by putting unnecessary stuff in the code. And if we really really need the crud it ought to be commented as such.

components/SideBar.vue

EmmaLRussell · 2024-09-10T15:47:34Z

layouts/default.vue

+appStore.initializeAppState();
+
+const setScreenSize = () => {
+  // As this function uses window, it can only be run client-side, so we have to pass the $pinia instance


Suggested change

// As this function uses window, it can only be run client-side, so we have to pass the $pinia instance

// As this function uses window, it can only be run client-side (post-setup), so we have to pass the $pinia instance

The linkage between these clauses wasn't immediately obvious to me!

I went through an adventure since writing this, submitted a PR to clarify the docs of the nuxt pinia module, and eventually the maintainer let me know that in fact "you don't need to pass the pinia instance within setup"
vuejs/pinia#2766

As such, the next PR removes this, here: https://github.com/jameel-institute/daedalus-web-app/pull/27/files#diff-433a9abd4ca70520f69fc064c160bb093918ce26dda6087a8bbfdbe2895d1944

EmmaLRussell · 2024-09-10T15:49:20Z

tests/e2e/runAnalysis.spec.ts

-  await page.click('div[aria-label="Advance vaccine investment"] label[for="medium"]');
+  await page.click('div[aria-label="Advance vaccine investment"] label[for="low"]');
+
+  await expect(page.getByRole("form")).toHaveAttribute("data-test-form-data", "{\"country\":\"United States\",\"pathogen\":\"influenza-1957\",\"response\":\"elimination\",\"vaccine\":\"low\"}");


I don't think we should be resorting to hidden attribute tests in e2e tests of forms - we should just be able to look at the values of the inputs.

We can be confident that the values of the inputs are what we just selected. I think the test to do for the e2e would be to wait until the result page is implemented, and test that the displayed parameters are correct.

Hang on, that doesn't look like the line (I thought) this was a comment for anymore, I'm pretty sure this was about the stringified JSON - or I thought it was!

Still looks like it's commenting on the stringified JSON to me!

Sorry I'm just blind as a 🦇

EmmaLRussell · 2024-09-10T15:58:55Z

tests/integration/rApi.spec.ts

+    it("returns a successful response when the mock server responds successfully", async () => {
+      const queryString = `parameters={"mockoonResponse":"successful","country":"Thailand","pathogen":"sars-cov-1","response":"no_closure","vaccine":"none"}`;
+
+      const response = await nuxtTestUtilsFetch(`/api/scenarios?${queryString}`, { method: "POST" });


Is this the test that was a problem for POSTing a body? I'm not completely sure, but it looks to me like the nuxt test utils uses ofetch: https://github.com/nuxt/test-utils/blob/f7afe52243a044694402fa16ddf51e80626133ab/src/core/server.ts#L4
..and that you should be able to include a POST body in the body option:
https://github.com/unjs/ofetch?tab=readme-ov-file#%EF%B8%8F-json-body

My problem had been that I was trying to pass objects into the 'body' property that just got lost in the post, as it were. Strings and objects evaporated. I eventually solved this by finding that I could wrap the parameters in a FormData object, which fetch does accept, and decompose that at the other end.

EmmaLRussell · 2024-09-10T16:06:29Z

tests/unit/components/ParameterForm.spec.ts

+    // I couldn't find a way to spy on the navigateTo function, so this is a second-best test that we
+    // at least construct the correct path to navigate to.


Did you try stubGlobal or mockNuxtImport?
https://stackoverflow.com/questions/77827093/how-to-check-if-navigateto-was-called-in-nuxt-js

OK, let's not hold up this branch, but it would be really good to find a way to mock global methods, and it seems really weird if the alleged mocking capacities of vitest/nuxt test utils aren't going to let us to that. Tbh if that proves impossible personally I'd rather not clutter up the code with extra props for the sake of unit tests, as long as the navigation etc gets tested in e2e tests. These components are going to be busy and confusing enough as it is, without a bunch of extra stuff that doesn't really need to be there.

EmmaLRussell

I think at some point we're going to have to work out how to POST JSON with Nuxt tests, as it really ought to be possible 😆 It's not terrible to post form values as form data but it would be nice to have the option not to..

See previous comment on the mocking navigate controversy - I'd rather take out the props that are just there for unit tests if it's really not possible to mock, and make sure the navigration is tested in e2e. I might have a fiddle around with the mocking, maybe I'll stumble on some other way of doing it... but that can go in another branch.

EmmaLRussell · 2024-09-11T14:17:27Z

components/ParameterForm.vue

+  const formDataObject = new FormData();
+  Object.entries(formData.value).forEach(([key, value]) => {
+    formDataObject.append(key, value.toString());
+  });


I'd assumed that we were just going to post JSON in the same format that the R API accepts and pass it straight through? You're saying that doesn't work (in the tests), that ordinary objects get turned into... what? Completely empty body?

It's not something to do with needing to set the content type is it? Does it default to POSTing form encoded data so it barfs on plain objects but accepts FormData? Hm, the ofetch docs suggest it should deal with that transparently and add the header itself, but it is supicious... Could try setting that header explicitly?

I have found something that appears to be working - JSON stringifying the body (in the test). The oftech docs claimed that this happened automatically, but evidently not 😕

It seems that still it behaves differently under test than in real life, so I had to add in a check for whether the body was being received as an object (as in real life, and in e2e tests) or a JSON parsable string (as in the integration test). I hope this is tolerable, as the price for moving away from FormData.

As discussed. maybe try adding header Content-Type: application/json when doing the POST.

EmmaLRussell · 2024-09-12T07:11:40Z

See previous comment on the mocking navigate controversy - I'd rather take out the props that are just there for unit tests if it's really not possible to mock, and make sure the navigration is tested in e2e. I might have a fiddle around with the mocking, maybe I'll stumble on some other way of doing it... but that can go in another branch.

This seemed to work for me: https://github.com/jameel-institute/daedalus-web-app/pull/29/files#

…uts-rebasing-onto-front-end-brach-test-fix fix integration tests

Jidea 57 mock navigate to 2

EmmaLRussell

OK this one's cooked as far as I'm concerned!

…ont-end-brach

david-mears-2 added 9 commits September 5, 2024 22:05

WIP 2: things related to post-form-submit, ie api and store

7d9c378

WIP. We dont have unit tests on the wip api endpoint + handler

02be36f

Remove store-related code

d1d4f9c

test playwright ci

7b146ba

Switch to using query params instead of body

52b12eb

This was necessitated because the nuxt test utils don't seem to have a way to pass through a body.

Tests

bcb53f2

Refactor: Use pinia store for largeScreen refs

7434c9d

Update e2e test

782a8db

remove comment

f45d8e9

Try to improve playwright test

12b7581

absternator reviewed Sep 6, 2024

View reviewed changes

david-mears-2 requested a review from absternator September 9, 2024 14:33

Base automatically changed from jidea-57-non-country-inputs-front-end to main September 9, 2024 15:18

Address PR comments

8c0c4f5

david-mears-2 requested a review from EmmaLRussell September 10, 2024 10:37

EmmaLRussell reviewed Sep 10, 2024

View reviewed changes

david-mears-2 added 4 commits September 10, 2024 18:09

Use body instead of query for req's to run scenario

0906bca

Temporary change to allow web app to be compatible with latest r api

0966d2b

Update e2e test to match latest R API build

143063c

Address PR comments

fbb2a05

david-mears-2 requested a review from EmmaLRussell September 10, 2024 23:48

EmmaLRussell reviewed Sep 11, 2024

View reviewed changes

david-mears-2 requested a review from EmmaLRussell September 12, 2024 10:56

Use JSON data for request bodies instead of FormData

2a03920

david-mears-2 force-pushed the jidea-57-non-country-inputs-rebasing-onto-front-end-brach branch from 6530321 to 2a03920 Compare September 12, 2024 12:14

M-Kusumgar and others added 3 commits September 12, 2024 20:53

fix integration tests

67ba34d

Merge pull request #30 from jameel-institute/jidea-57-non-country-inp…

c3a4d8e

…uts-rebasing-onto-front-end-brach-test-fix fix integration tests

test changes

fbc712e

EmmaLRussell and others added 2 commits September 13, 2024 13:43

parse body in test

93ca120

Merge pull request #33 from jameel-institute/jidea-57-mock-navigateTo-2

501d92a

Jidea 57 mock navigate to 2

EmmaLRussell approved these changes Sep 13, 2024

View reviewed changes

david-mears-2 and others added 2 commits September 13, 2024 16:18

Merge branch 'main' into jidea-57-non-country-inputs-rebasing-onto-fr…

d84c44c

…ont-end-brach

Update e2e test to use real test run ids

3e43583

david-mears-2 merged commit cdb8997 into main Sep 13, 2024
6 checks passed

david-mears-2 deleted the jidea-57-non-country-inputs-rebasing-onto-front-end-brach branch September 13, 2024 15:41

	navigateToData.value = `/scenarios/${runId}`;
	await navigateTo(`/scenarios/${runId}`)


		const modelParameters = JSON.parse(query.parameters as string) as ParameterDict;

		// Delegate to runScenario so that the logic can be unit-tested.


		const rApiNewScenarioEndpoint = "/scenario/run";

		// Send a request to the R API for a job run.

	// As this function uses window, it can only be run client-side, so we have to pass the $pinia instance
	// As this function uses window, it can only be run client-side (post-setup), so we have to pass the $pinia instance

		// I couldn't find a way to spy on the navigateTo function, so this is a second-best test that we
		// at least construct the correct path to navigate to.

Make run button functional #25

Make run button functional #25

Conversation

david-mears-2 commented Sep 6, 2024

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

absternator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell commented Sep 12, 2024

EmmaLRussell left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 6, 2024 •

edited

Loading

david-mears-2 Sep 6, 2024 •

edited

Loading

david-mears-2 Sep 10, 2024 •

edited

Loading

david-mears-2 Sep 10, 2024 •

edited

Loading

david-mears-2 Sep 10, 2024 •

edited

Loading

david-mears-2 Sep 12, 2024 •

edited

Loading