add tests for several packages #1024

43081j · 2020-03-21T20:59:44Z

self-isolation lead to me writing some tests for you.

if they need changing any way just let me know.

cc @e111077

I'll fix the lint error tomorrow, failing ci atm

packages/menu/src/test/mwc-menu.test.ts

e111077 · 2020-03-21T23:01:18Z

lint errors can be fixed via npm run lint:typescript -- --fix

e111077

There is a lot of excellent work that I didn't comment on because then you'd have to "resolve" the comment. Very very excellent work!

A few standardization comments. I really wish we could keep you locked up after all this stuff ends because you just saved me so much time haha!

packages/drawer/src/test/mwc-drawer.test.ts

packages/fab/src/test/mwc-fab.test.ts

packages/menu/src/test/mwc-menu-surface.test.ts

packages/menu/src/test/mwc-menu.test.ts

packages/slider/src/test/mwc-slider.test.ts

e111077 · 2020-03-21T23:30:24Z

What was your dev setup btw? I need to write a contributing / developing doc soon. I usually have 4 terminal windows running:

npm run watch
npm run watch:tests
npm run test:debug -- --packages <package name>

and an empty one to run whatever command like npm run format or npm run lint -- --fix

43081j · 2020-03-21T23:45:36Z

I have my pixel book at the min so had to fiddle with the setup a bit to get Chrome to launch (I installed puppeteer and set CHROME_BIN to its executable path).

But other than that, I have been running a build and test manually:

npm run build:typescript && npm run test -- --packages=THE_ONE_IM_WORKING_ON

i didn't realise a watch script was already set up as I wasn't running the commands much. I'd use that just the way you've said now that I know

And before committing, I'd run format and lint (though clearly I forgot in my last push 😂)

packages/button/src/test/mwc-button.test.ts

43081j · 2020-03-23T21:26:45Z

FYI @e111077 i dropped sinon but kept their timers package (as you can use it standalone) for mocking setTimeout. but it turns out their build isn't a valid ES module even though its in the "module" field of their package.json, so ill push it in a branch on top of this branch until sinonjs/fake-timers#320 is merged.

e111077

REALLY excellent work, again

packages/button/src/test/mwc-button.test.ts

packages/drawer/src/test/mwc-drawer.test.ts

packages/menu/src/test/mwc-menu.test.ts

e111077 · 2020-03-30T22:04:37Z

test/src/util/helpers.ts

+export const waitForEvent = (el: Element, ev: string) => new Promise((res) => {
+  el.addEventListener(ev, () => {
+    res();
+  }, {once: true});


also surprised this is compiled out correctly by es-dev-server

e111077

brilliant, brilliant, brilliant, absolutely brilliant work!

Approving, but I just want some clarification on the sinon part

e111077 · 2020-03-30T22:22:09Z

migrating internally to run google-wide tests

43081j · 2020-03-31T18:24:40Z

here you are. i added sinon to the one test, ill do a new pr once my sinon one gets merged (which reminds me i need to go finish it...)

e111077 · 2020-03-31T18:37:34Z

LGTM. Need to make some build changes internally to make this work. Requesting a code freeze on this PR in the meanwhile. Thanks!

e111077

Heya, sorry to double back. I think for the sake of getting this submitted can you please remove sinon fake timers? The internal build system does not like it and cannot actually find SinonFakeTimers. It seems for some reason sinon types never actually resolve in our tests and just use the untyped js file.

e111077 · 2020-04-01T20:00:46Z

Thanks!

43081j · 2020-04-01T20:00:52Z

@e111077 thats too bad. though it doesn't surprise me, their 'browser bundle' is a little hacked together i'd say...

i just deleted sinon to solve it. i think we should still make sure we introduce a utility function at least for mocking timers, but this can probably do for now.

e111077 · 2020-04-01T22:19:14Z

can you please file an issue asking for the timers helper so we make sure it doesn't slip through the cracks

e111077 · 2020-04-02T01:17:05Z

had to make a few IE-specific changes. Have to figure out why IE tests aren't running externally.

Copybara service will forward these changes when it is merged internally. It usually closes this PR as well. If it doesn't it should still carry your authored commits into master.

@e111077