Mrc 6896 tests pt 7 - fix bugs and e2e tests#277
Mrc 6896 tests pt 7 - fix bugs and e2e tests#277M-Kusumgar wants to merge 3 commits intomrc-6896-new-test-6from
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mrc-6896-new-test-6 #277 +/- ##
=======================================================
- Coverage 98.70% 98.53% -0.18%
=======================================================
Files 183 183
Lines 4554 4566 +12
Branches 1010 1018 +8
=======================================================
+ Hits 4495 4499 +4
- Misses 47 54 +7
- Partials 12 13 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| <wodin-tabs id="right-tabs" | ||
| :tab="tab" | ||
| :tabNames="rightTabNames" | ||
| @tabSelected="rightTabSelected"> |
There was a problem hiding this comment.
you will see this change in all the tabs. this is because we need to "control" wodin tab and make it mirror the store state instead of its internal state for the right tabs (where the graphs are).
if we dont sync the internal state of wodin tabs with the store then our store in the graphs state could think the visible graph groups are "Fit" for example (remember visible graph groups are from the open tab) and wodin tabs may think the tab that is selected is "Run" so when the user loads a session then the store generates a graph for the fit tab but the run tab is displayed, meaning its basically a blank graph
| // multi sensitivity tab does not actually care about what variables are selected | ||
| // so select them all otherwise wodin will raise no variables selected error | ||
| const allSelectedVariables = computed(() => | ||
| multiSensitivity | ||
| ? store.state.model.variablesCopy | ||
| : getAllSelectedVariables(store.state) | ||
| ); |
There was a problem hiding this comment.
turns out multi sens doesnt care what variables are selected which makes sense, it should just give the data for all the variables
| const selectedTabName = ref(props.tab || props.tabNames[0]); | ||
|
|
||
| const tabSelected = (tabName: string) => { | ||
| selectedTabName.value = tabName; | ||
| if (!props.tab) selectedTabName.value = tabName; | ||
| emit("tabSelected", tabName); | ||
| }; | ||
|
|
||
| watch(() => [props.tab], ([newTab]) => { | ||
| if (newTab) selectedTabName.value = newTab; | ||
| }); | ||
|
|
There was a problem hiding this comment.
if props.tab exists we make it watch the prop and update in sync with it, the best way to do this is to just refactor this component so it always mirrors the store but it is also used for the left tabs ("Options", "Code", "Data") which would require a bit more work and this refactor has gone on for long enough
| odinModelCodeError: model.odinModelCodeError, | ||
| paletteModel: model.paletteModel | ||
| paletteModel: model.paletteModel, | ||
| variablesCopy: model.variablesCopy, |
There was a problem hiding this comment.
skipped on serialising this which caused errors, it was very difficult to spot this because our serialisation types for states are declared as new types instead of being derived from the actual state types, i think at some point we should make them all derived from the actual state to avoid bugs like this
| persisted: true | ||
| }); | ||
|
|
||
| targetState.graphs.visibleData = {}; |
There was a problem hiding this comment.
this needs to be added back in as an empty obj as graph state doesnt serialise this
| const yAxis = page.locator(`g[id^="y-axes"]`); | ||
| const firstTick = yAxis.locator(".tick").first(); | ||
| await expect(await firstTick.textContent()).toBe("100p"); | ||
| await expect(firstTick).toHaveText("100p"); |
There was a problem hiding this comment.
graph updates are async with the plugin we wrote to update the data so we need to use playwright's auto retrying assertions like ".toHaveText"
|
|
||
| // Check run plot | ||
| await page.click(":nth-match(.wodin-right .nav-tabs a, 1)"); // Run tab | ||
| await page.waitForTimeout(50); |
There was a problem hiding this comment.
these 3 waits are the only functionality changes i made to this test
There was a problem hiding this comment.
i had to scratch an itch that was bothering me for a while with this file (and e2e tests in wodin), most of the playwright tests in wodin are written incorrectly and my linter in my editor goes mental but also i go a bit mental seeing the await spam, await a page locator has no effect and await expect(...).<function>() is only a thing when the <function> is a playwright auto retrying assertion, it will not auto retry if we write await expect(...).toBe(...) for example so i fixed it in this file, i may fix this slowly over future tickets whenever the opportunity arises
| it("renders Sensitivity as expected", async () => { | ||
| const wrapper = getWrapper(); | ||
| const rightTabs = wrapper.find("#right-tabs"); | ||
|
|
||
| // Change to Sensitivity tab | ||
| await rightTabs.findAll("li a").at(1)!.trigger("click"); | ||
| expect(rightTabs.find("div.mt-4 button").text()).toBe("Run sensitivity"); | ||
| }); |
There was a problem hiding this comment.
it no longer changes tabs without changing it in the store so this test would be equivalent to the next test where we test that clicking a tab calls mockSetOpenTab, this is the same for the other tests ive deleted in fitApp.test.ts and stochasticApp.test.ts
1 participant
This should fix all the e2e tests. There are a couple of random bug fixes so ive left comments in the PR for explanations
ive also added a short plugin test that i forgot to add earlier