-
Notifications
You must be signed in to change notification settings - Fork 1
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
JIDEA-168: component tests for globe #93
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
===========================================
+ Coverage 1.44% 37.15% +35.70%
===========================================
Files 55 56 +1
Lines 146190 146233 +43
Branches 327 371 +44
===========================================
+ Hits 2116 54334 +52218
+ Misses 144057 91883 -52174
+ Partials 17 16 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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 stuff. Mostly questions!
|
||
// Extract the map object from the file content | ||
// eslint-disable-next-line no-eval | ||
const map = eval(fileContent.replace("export default", "")); |
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.
I guess you could do this as a dynamic import. But it doesn't really matter!
const updatedFileContent = `const map = ${JSON.stringify(map, null, 2)};\nexport default map;`; | ||
|
||
// Save the updated content back to the file | ||
fs.writeFileSync(filePath, updatedFileContent, "utf8"); |
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.
I guess it would be a little more cautious to write out to a different file from the input file.
@@ -0,0 +1,6 @@ | |||
module.exports = { |
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.
Why is this file needed?
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.
Because this says so: https://www.amcharts.com/docs/v5/getting-started/integrations/jest/
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. Might be worth sticking in a comment to that effect as I for one will definitely forget that.
tests/unit/components/Globe.spec.ts
Outdated
// NB: These component tests perform assertions about the return value of setUpChart(), which I call | ||
// directly in the tests. This is a second-best kind of test, since it means we're testing a | ||
// second chart object, rather than the one that was created automatically by the component | ||
// setup script. I do this because I couldn't find a way to access that first chart object from here. |
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.
Could you set the return object as a value in the component and then access it via vm
? setupChart
does return the chart, but it doesn't look like we do anything with it (except in the test).
Or is the problem that setupChart
isn't actually being called in the test (the globediv
watch isn't being triggered?
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.
It runs setupChart twice (in the case of running a test) - once on setup and once when we call it directly from the test. (As confirmed by console logging)
What do you mean by setting it 'as a value'? When I access 'component.vm.chart' it is mysteriously undefined. (In fact, it's whatever we initialize 'chart' to, since component.vm.chart is 3 when we initialize chart to 3).
Anyway, I've now found that if I swap out 'mountSuspended' (@nuxt/test-utils) to 'mount' (@vue/test-utils) the problem goes away.
tests/unit/components/Globe.spec.ts
Outdated
expect(chart.get("rotationX")).toBe(-100); | ||
expect(chart.get("rotationY")).toBe(-25); |
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.
Comments on these magic numbers would be nice too. -25 is amountToTileEarthUpBy? -100 is Thailand-centric default?
tests/unit/components/Globe.spec.ts
Outdated
expect(chart.series._values.length).toBe(9); | ||
const gbrSeries = chart.series._values[8]; |
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.
Series are sort of like layers? And here you're expecting the highlighted one to be the final one?
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.
Yes, a newly added series
tests/unit/components/Globe.spec.ts
Outdated
// Ensure that the name has been updated from 'United Kingdom of Great Britain and Northern Ireland' to 'United Kingdom' | ||
// Name is used for tooltips. | ||
expect(gbrSeries._settings.geoJSON.properties.name).toBe("United Kingdom"); |
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.
Where do these updated names come from?
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.
They come from the metadata:
const modelCountryName = appStore.globeParameter?.options?.find(o => o.id === feature.id)?.label;
tests/unit/components/Globe.spec.ts
Outdated
|
||
expect(component.vm.gentleRotateAnimation.stopped).toBe(true); | ||
|
||
const gbrSeries = chart.series._values[8]; |
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.
Might be good to make a helper to select what should be the highlighted series (always the last one?) as this crops up a couple of times,
tests/unit/components/Globe.spec.ts
Outdated
await waitFor(() => { | ||
expect(gbrSeries._settings.fill).not.toBe(originalColor); | ||
expect(chart.get("rotationX")).toBe(originalX); | ||
expect(chart.get("rotationY")).toBe(originalY); | ||
}); |
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.
So it should update the colour but shouldn't update the rotation. And by the time it updates the colour it would have updated the rotation if it was going to? So doing this as a waitFor is ok..?
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.
I believe the test is valid, but to make it more robust to any future changes to the component, I've added a line that expects the fill color to be the target color, not just 'different from the original'.
Fix globe component
Swapping mountSuspended for mount (from vue test utils) solved the issue described in the deleted comment, where 'component.vm.chart' was always returning the value with which the component initialized the 'chart' variable.
Test is failing because of API container image |
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.
Looks good, would just suggest adding a comment to the babel file.
remove API configure step from working globe branch
Here are tests for the main testable behaviours of the globe component. As the component draws on a canvas, and the chart library doesn't expose functions like 'click', I couldn't find a way to simulate users actually clicking / hovering countries. So I mainly test the results of updating things in the store that the component has a watcher on, which correspond to user interactions.
Reference: https://www.amcharts.com/docs/v5/getting-started/integrations/jest/ I found that the list of libraries mentioned here was necessary.
NB: These component tests perform assertions about the return value of setUpChart(), which I call directly in the tests. This is a second-best kind of test, since it means we're testing a second chart object, rather than the one that was created automatically by the component setup script. I do this because I couldn't find a way to access that first chart object from here:
component.vm.chart
was always null.For the reasons outlined in shapefiles.md (ie to avoid unit tests polluting the state of the geodata for each other), I introduce a script in this PR that reverses the winding order of the geodata (to be run when geodata is added to the repo) into the winding order used by amcharts (which is the reverse of WHO's winding order).