Moved from jest to jasmine-karma #189

matiasbavera · 2020-12-18T17:57:08Z

What's new

Moved react-components tests from jest/jsdom to jasmine/karma.

Observations:

Changed .test extension to .spec for karma to work.
Syntax of the assertion library changed (we are using jasmine now).
We lost the jest.fn(). But we can use an empty function and spyOn.
Added a karma config file and a Webpack configuration file for pre-processing the project's files. There is a typescript-karma compiler but it's not working great when we deal with third-party libraries and CSS. Webpack do the pre-processing for us and it's more flexible so, I opted to go with Webpack.
Added puppeteer to install and open a headless chrome in Github's actions server, for testing with Karma.

Self-checks

I'm familiar with and follow this Typescript guideline
I added unit-tests for new components
I tried testing edge cases
I tested the behavior of the components that interact with the backend, with an e2e test

matiasbavera · 2020-12-18T20:20:39Z

Hi @koonpeng, I moved and ran all the react-component tests with Jasmine and Karma. Now I am fighting to integrate the coverage test.

matiasbavera · 2020-12-21T19:22:01Z

We have coverage now.

matiasbavera · 2020-12-21T19:27:47Z

We have this warning:

It looks like there are several instances of @material-ui/styles initialized in this application.
This may cause theme propagation issues, broken class names, specificity issues, and makes your application bigger without a good reason.

It's an open issue.
mui/material-ui#15745

https://material-ui.com/getting-started/faq/#i-have-several-instances-of-styles-on-the-page

alias: {
"@material-ui/styles": path.resolve(__dirname, "node_modules", "@material-ui/styles"),
}


ERROR in /home/ekumen/git/osrf/romi-dashboard/node_modules/@material-ui/core/esm/styles/makeStyles.js
Module not found: Error: Can't resolve '@material-ui/styles' in '/home/ekumen/git/osrf/romi-dashboard/node_modules/@material-ui/core/esm/styles'

matiasbavera · 2020-12-21T19:39:29Z

If we want html coverage reports we should use "istanbul-instrumenter-loader": "^0.2.0",

webpack-contrib/istanbul-instrumenter-loader#32
gotwarlost/istanbul#660
karma-runner/karma-coverage#251

matiasbavera · 2020-12-21T21:45:09Z

Having this error on the CI


Unable to detect ROS, please make sure a supported version of ROS is sourced
gyp: Call to 'node scripts/ros_distro.js' returned exit status 1 while in binding.gyp. while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (/opt/hostedtoolcache/node/12.20.0/x64/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:351:16)
gyp ERR! stack     at ChildProcess.emit (events.js:314:20)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12)
gyp ERR! System Linux 5.4.0-1032-azure
gyp ERR! command "/opt/hostedtoolcache/node/12.20.0/x64/bin/node"

Solved

codecov · 2020-12-22T00:18:15Z

Codecov Report

Merging #189 (7e5ae6c) into master (c2480a1) will decrease coverage by 4.81%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
- Coverage   69.25%   64.44%   -4.82%     
==========================================
  Files         107       92      -15     
  Lines        2885     2607     -278     
  Branches      605      485     -120     
==========================================
- Hits         1998     1680     -318     
+ Misses        880      820      -60     
- Partials        7      107     +100

Flag	Coverage Δ
dashboard	`57.96% <ø> (ø)`
react-components	`74.13% <100.00%> (-8.47%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...onents/lib/task-summary/task-summary-accordion.tsx	`92.00% <100.00%> (-0.86%)`	⬇️
...eact-components/lib/commands/loop-request-form.tsx	`73.62% <0.00%> (-22.08%)`	⬇️
...ges/react-components/lib/robots/default-marker.tsx	`80.76% <0.00%> (-19.24%)`	⬇️
packages/react-components/lib/simple-info.tsx	`71.11% <0.00%> (-18.89%)`	⬇️
...react-components/lib/waypoints/waypoint-marker.tsx	`84.61% <0.00%> (-15.39%)`	⬇️
...ages/react-components/lib/lifts/lift-accordion.tsx	`65.90% <0.00%> (-14.87%)`	⬇️
...-components/lib/commands/delivery-request-form.tsx	`82.20% <0.00%> (-14.63%)`	⬇️
...kages/react-components/lib/spotlight-accordion.tsx	`84.61% <0.00%> (-11.82%)`	⬇️
...-components/lib/dispensers/dispenser-accordion.tsx	`62.50% <0.00%> (-10.97%)`	⬇️
...ackages/react-components/lib/lifts/lift-marker.tsx	`65.90% <0.00%> (-8.64%)`	⬇️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2480a1...7e5ae6c. Read the comment docs.

matiasbavera · 2020-12-22T20:01:25Z

@koonpeng Ready for first comments. =)

koonpeng

Can you make another PR (or in this PR) to see if we can replace the smoke tests with screenshot snapshot diffing?

packages/react-components/lib/task-summary/task-summary-accordion.tsx

packages/react-components/package.json

packages/react-components/tests/dispensers/dispenser-marker.spec.tsx

packages/react-components/tests/omni-panel.spec.tsx

koonpeng · 2020-12-28T02:34:31Z

packages/react-components/tests/robots/robot-marker.spec.tsx

+    // TextEncoder is not available in node
+    colorManager = new ColorManager();
+    handler = {
+      onClick: () => console.log('mock'),
+      robotColor: async () => 'black',
+    };
+
+    spyOn(handler, 'onClick');
+    spyOn(handler, 'robotColor');
+
+    colorManager.robotPrimaryColor = handler.robotColor;


Since we are running the tests in browser, this comment shouldn't be valid anymore. There also shouldn't be a need to mock ColorManager now.

Had a lot of errors without instantiating ColorManager.

ERROR in [at-loader] ./tests/robots/robot-marker.spec.tsx:23:32 TS2739: Type 'Spy<Func>' is missing the following properties from type 'ColorManager': conflictHighlight, robotPrimaryColor, robotColorFromCache, _robotColorFromId, _robotColorCache ERROR in [at-loader] ./tests/robots/robot-marker.spec.tsx:32:32 TS2322: Type 'Spy<Func>' is not assignable to type 'ColorManager'.

Also, I think we need that for this test to work:

colorManager.robotPrimaryColor = jasmine.CreateSpy(); it("uses ColorManager to determine robot's color", async () => { await render(<RobotMarker robot={makeRobot()} fleetName="test_fleet" footprint={1} />); expect(colorManager.robotPrimaryColor).toHaveBeenCalled(); });

packages/react-components/tests/test-utils.ts

packages/react-components/tsconfig.json

koonpeng · 2020-12-28T02:49:40Z

We have this warning:

It looks like there are several instances of @material-ui/styles initialized in this application.
This may cause theme propagation issues, broken class names, specificity issues, and makes your application bigger without a good reason.

Since we are using a monorepo, the @material packages are in the root package to avoid multiple instances being loaded. Maybe we need to change the webpack/karma config so it bundles correctly? Or maybe this warning comes from the way karma load tests, resulting in each test creating its own @material/styles instance?

koonpeng · 2020-12-28T02:52:03Z

packages/react-components/package.json

@@ -13,28 +13,49 @@
    "clean": "tsc --build --clean",
    "test": "jest --watch",
    "test:coverage": "jest --ci --coverage",
+    "test:karma": "./node_modules/karma/bin/karma start karma.conf.js",


nit:
Is it possible to use the karma cli?

We can use it if we install karma-cli globally, but I think that for that reason, dependency should be outside the package.json. Let me know what do you think is the best aproach

matiasbavera · 2021-01-11T19:47:24Z

Alright, I'll open a new PR

koonpeng · 2021-01-12T02:28:21Z

Looks like this got nuked when we change master to main, go ahead and open a new PR!

matiasbavera added 2 commits December 21, 2020 18:46

Moved from jest to jasmine-karma

b9c1cf4

Updated tests, coverage tests working

0521e64

matiasbavera force-pushed the experimental/karma branch from b29eece to 0521e64 Compare December 21, 2020 21:46

matiasbavera added 6 commits December 21, 2020 18:52

activated codecov

78fa02f

updated package-lock.json

597897a

Reverted json-lock dashboard

ca90e85

Updated package-lock react component

515896d

updated dashboard login snapshot

1a91dbd

removed ros2-bridge

d3b338f

matiasbavera added 12 commits December 22, 2020 09:48

increased fs.inotify.max_user_watches

068f286

updated pull-request.yml

a74a6e8

Disabled coverage tests

51a502f

Added puppeteer, single run = true

33ebba8

Added codecov as a step

ed693b5

Added types and comments

ea38622

Removed gh-pages

2768ce5

Removed webpack leftover

575ce28

Added file to coverage report

5759c67

test coverage

3316868

coverage reports on root

8d221e5

Removed json report and uncommented pull-request.yml

7e5ae6c

matiasbavera requested a review from koonpeng December 22, 2020 20:01

matiasbavera marked this pull request as ready for review December 22, 2020 20:01

koonpeng requested changes Dec 28, 2020

View reviewed changes

koonpeng reviewed Dec 28, 2020

View reviewed changes

koonpeng closed this Dec 29, 2020

koonpeng deleted the branch open-rmf:master December 29, 2020 02:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved from jest to jasmine-karma #189

Moved from jest to jasmine-karma #189

matiasbavera commented Dec 18, 2020 •

edited

Loading

matiasbavera commented Dec 18, 2020

matiasbavera commented Dec 21, 2020

matiasbavera commented Dec 21, 2020 •

edited

Loading

matiasbavera commented Dec 21, 2020 •

edited

Loading

matiasbavera commented Dec 21, 2020 •

edited

Loading

codecov bot commented Dec 22, 2020 •

edited

Loading

matiasbavera commented Dec 22, 2020 •

edited

Loading

koonpeng left a comment

koonpeng Dec 28, 2020

matiasbavera Jan 28, 2021

koonpeng commented Dec 28, 2020

koonpeng Dec 28, 2020

matiasbavera Jan 28, 2021 •

edited

Loading

matiasbavera commented Jan 11, 2021

koonpeng commented Jan 12, 2021

Moved from jest to jasmine-karma #189

Moved from jest to jasmine-karma #189

Conversation

matiasbavera commented Dec 18, 2020 • edited Loading

What's new

Self-checks

matiasbavera commented Dec 18, 2020

matiasbavera commented Dec 21, 2020

matiasbavera commented Dec 21, 2020 • edited Loading

matiasbavera commented Dec 21, 2020 • edited Loading

matiasbavera commented Dec 21, 2020 • edited Loading

codecov bot commented Dec 22, 2020 • edited Loading

Codecov Report

matiasbavera commented Dec 22, 2020 • edited Loading

koonpeng left a comment

Choose a reason for hiding this comment

koonpeng Dec 28, 2020

Choose a reason for hiding this comment

matiasbavera Jan 28, 2021

Choose a reason for hiding this comment

koonpeng commented Dec 28, 2020

koonpeng Dec 28, 2020

Choose a reason for hiding this comment

matiasbavera Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

matiasbavera commented Jan 11, 2021

koonpeng commented Jan 12, 2021

matiasbavera commented Dec 18, 2020 •

edited

Loading

matiasbavera commented Dec 21, 2020 •

edited

Loading

matiasbavera commented Dec 21, 2020 •

edited

Loading

matiasbavera commented Dec 21, 2020 •

edited

Loading

codecov bot commented Dec 22, 2020 •

edited

Loading

matiasbavera commented Dec 22, 2020 •

edited

Loading

matiasbavera Jan 28, 2021 •

edited

Loading