search

coveo / ui-kit

Coveo UI kit repository, home of @coveo/headless, @coveo/atomic, and more.
Apache License 2.0
52 stars 34 forks source link

[BB pr#48] KIT-49: First commit to setup Cypress for Atomic #48

Closed coveobot closed 3 years ago

coveobot commented 4 years ago

Pull request :twisted_rightwards_arrows: created by bitbucket user Lucille Vu on 2020-06-19 13:23 Last updated on 2020-06-25 20:38 Original Bitbucket pull request id: 48

Participants:

  • @acote-coveo (reviewer)
  • bitbucket user Lucille Vu
  • @ThibodeauJF (reviewer) :heavy_check_mark:
  • @btaillon (reviewer)
  • bitbucket user coveo-anti-throttling_02
  • @olamothe (reviewer) :heavy_check_mark:
  • @samisayegh (reviewer) :heavy_check_mark:

Source: Commit ad046d82fc6b on branch KIT-49_Cypress_4_Atomic> Destination: https://bitbucket.org/coveord/ui-kit/commits/1df2414f4b2e on branch master Merge commit: https://github.com/None/commit/7ce5565da1a4

State: MERGED

What includes:

‌

coveobot commented 4 years ago

@samisayegh commented on 2020-06-19 13:43

Outdated location: line 18 of packages/atomic/cypress/README.md

I think it’s nicer to make these npm scripts in the package.json similar to cypresstest. Rather the document the full command, I would put the shorter npm script in the README.md. e.g.

npm run cypress:open
npm run cypress:run

// package.json
"cypress:run" : npx cypress run --project <path>

‌

Also, I’m curious, what does “cypress open” do?

coveobot commented 4 years ago

@samisayegh commented on 2020-06-19 13:45

Outdated location: line 6 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

Is there a dev dependency for this polyfill we could install versus checking in the file into version control?

coveobot commented 4 years ago

@samisayegh commented on 2020-06-19 13:47

Outdated location: line 17 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

What are these “route” lines doing?

coveobot commented 4 years ago

@samisayegh commented on 2020-06-19 13:48

Outdated location: line 46 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

Why do we get these elements in the beforeEach?

Edit: I see they are referenced below using the @? Why do this instead of retrieve the element in the test? Is it for performance reasons?

Also, what if one test changes the state of the element? e.g. adds text to the search box. Does it get reset?

‌

coveobot commented 4 years ago

@samisayegh commented on 2020-06-19 13:52

Outdated location: line 34 of packages/atomic/package.json

Interesting, seems like cypress is built on top of mocha?

coveobot commented 4 years ago

@samisayegh commented on 2020-06-19 14:02

How did you find writing the tests? Was it more/less/just as enjoyable as other technologies?

I noticed there is some nesting. Could that be flattened using async/await syntax?

Cypress tooling aside, I think it would also be good to compare cypress test-writing to stencil’s puppeteer. I adapted the first example from their docs and find it really clean. At the same time, maybe it’s possible to write cypress code that looks just like it, that mimics the newE2EPage utility?

import { newE2EPage } from '@stencil/core/testing';

describe('example', () => {
  it('should render an atomic-search-box', async () => {
    const page = await newE2EPage();
    await page.setContent(`<atomic-search-box></atomic-search-box>`);
    const el = await page.find('atomic-search-box');
    expect(el).not.toBeNull();
  });
});

‌

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-19 14:02

Outdated location: line 6 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

we can load the file directly from https://unpkg.com/unfetch/dist/unfetch.umd.js. I’m not sure if it’s better?

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-19 14:04

Outdated location: line 17 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

this route is called in order to interact with the Network Console. If the test doesn’t need to interact with those api/ua calls, we don’t have to add it. The test I tried below has some parts to interact with NetworkConsole, so I added them

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-19 14:07

Outdated location: line 46 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

I still don’t know which is the best practice to organize selector for Cypress, I put it in BeforeEach, so I don’t have to declare it later in the test below (if needed). Currently I use very ‘general’ selectors, so no, it won’t be affected by the state of the element

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-19 14:21

How did you find writing the tests? Was it more/less/just as enjoyable as other technologies? _ Not much complicated, but I don’t know at this moment which way will be the best practice (write code, optimize, organize the selectors for easily maintenance in the future)

I noticed there is some nesting. Could that be flattened using async/await syntax?_ I think yes, but I never try, 1 of the + point for cypress compare to Nightwatch, is they support Promise

_Cypress tooling aside… __I will doubt if stencil allows us to interact with the NetworkConsole, that’s one important requirement. I checked their e2e testing, not many features indeed.

Last part, i think yes it’s possible, but I never try before, (maybe like this https://stackoverflow.com/questions/52258630/cypress-and-script-injection-inside-test-scenario)

‌

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-19 17:22

Outdated location: line 34 of packages/atomic/package.json

yes, https://docs.cypress.io/guides/references/bundled-tools.html#Mocha

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-19 17:47

Here is the way to add code into html

cy.document().then((document) => {
      document.body.innerHTML =
        '<atomic-search-box></atomic-search-box><atomic-result-list></atomic-result-list>';
    });

‌

coveobot commented 4 years ago

@acote-coveo commented on 2020-06-19 17:53

Outdated location: line 6 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

It will be available for the next release: https://github.com/cypress-io/cypress/issues/95#issuecomment-646056343

Loading the file directly is a bad IDEA as it won’t be cached and you’ll have to download it every time.

There’s also that plugin that could be used temporally: https://github.com/RcKeller/cypress-unfetch

coveobot commented 4 years ago

@olamothe commented on 2020-06-22 12:42

Outdated location: line 46 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

A proposition for structure:

‌

cypress/fixtures/...
cypress/plugins/...
cypress/support/...

// All selectors related to each components exported as pure function here.
cypress/selectors/searchbox/...
cypress/selectors/facet/...
cypress/selectors/sort/...

// All tests related to each component exported here.
// Multiple file for each component, with each file focusing on a task.
cypress/integration/searchbox/...
cypress/integration/sort/...
cypress/integration/facet/...

‌

coveobot commented 4 years ago

@olamothe commented on 2020-06-22 12:44

Outdated location: line 6 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

@{557058:16667da0-b907-4ddf-8135-42c751843f9f} I would investigate the plugin.

Or at least leave a todo to remove the polyfill when available officially from cypress.

coveobot commented 4 years ago

@olamothe approved :heavy_check_mark: the pull request on 2020-06-22 12:45

coveobot commented 4 years ago

@olamothe commented on 2020-06-22 12:46

As a general comment, I would recommend every test be rewritten with async/await.

Makes it much more readable/less code nesting.

‌

Also, @{557058:16667da0-b907-4ddf-8135-42c751843f9f} , can you share with us links/dashboard that shows the test run in Cypress ? Or share screenshots with us if you can’t.

I’m also interested in seeing if we could have these run on every PR/if there’s some kind of integration we could have with Bitbucket (until the whole company moves to github).

The tooling aspect of this is super important.

coveobot commented 4 years ago

@olamothe commented on 2020-06-22 12:49

Outdated location: line 49 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

Try to have the describe and it naming follow a “natural sentence format”. It will be just a bit nicer when we read the test output in the end.

‌

For example:

‌

Searchbox should load
Searchbox should show query suggestions
Searchbox should execute a query on button click
Searchbox should call usage analytics when executing a query
etc..

‌

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-22 17:31

Outdated location: line 6 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

Yes, I’m aware about #95, and upcoming support for that. And I aslo feel I prefer to wait for official support (even with experimental tag)

Recently, what I used is theirs receipt to workaround for now as well
https://github.com/cypress-io/cypress-example-recipes/blob/master/examples/stubbing-spying__window-fetch/cypress/integration/polyfill-fetch-from-tests-spec.js

‌

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:30

Outdated location: line 26 of README.md

To run all FTs using Cypress

Since it will run Cypress in all sub repos.

Maybe you can also add a note akin to the one for dev:

**Note:** You should build all separate projects at least once before running tests.

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:46

Outdated location: line 1 of packages/atomic/cypress/README.md

Why not add this section in the README at the atomic level, so it would be more visible.

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:48

Outdated location: line 12 of packages/atomic/cypress/README.md

If you move it, then it should be npm run cypressopen

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:48

Outdated location: line 18 of packages/atomic/cypress/README.md

If you move it, then it should be npm run cypresstest

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:48

Outdated location: line 21 of packages/atomic/cypress/README.md

If you move it, you can remove this section

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:49

Outdated location: line 10 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

“SearchBox Test” be more descriptive

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:50

Outdated location: line 36 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

Is that subsection necessary?

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:53

Outdated location: line 37 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

if the first describe is “SearchBox”

then you wouldn’t need to rewrite SearchBox for every “it”

‌

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:57

Location: line 1 of packages/atomic/src/pages/test.html

Maybe you can leverage this folder for testing the SearchBox

Have a file SearchBox.test.html with only it as a component

coveobot commented 4 years ago

@ThibodeauJF approved :heavy_check_mark: the pull request on 2020-06-23 17:57

coveobot commented 4 years ago

@ThibodeauJF commented on 2020-06-23 17:58

Seems there’s an issue with Mocha types failing the build, are they really necessary?

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-23 17:59

Outdated location: line 1 of packages/atomic/cypress/README.md

Ok, I will move it to README at Atomic level

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-23 18:16

Outdated location: line 36 of packages/atomic/cypress/integration/searchBox.sample.spec.ts

no, not necessary at this moment

coveobot commented 4 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-06-23 20:06

Bundle Size Report

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 127.6 118 -7.5
minified 64.7 59.2 -8.5
gzipped 20.4 18.9 -7.1
dist/browser/headless.esm.js bundled 123.7 114.5 -7.5
minified 72.5 65.6 -9.5
gzipped 21.2 19.7 -7.1
coveobot commented 4 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-06-23 20:59

Bundle Size Report

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 127.6 118 -7.5
minified 64.7 59.2 -8.5
gzipped 20.4 18.9 -7.1
dist/browser/headless.esm.js bundled 123.7 114.5 -7.5
minified 72.5 65.6 -9.5
gzipped 21.2 19.7 -7.1
coveobot commented 4 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-06-23 21:18

Bundle Size Report

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 127.6 119.8 -6.1
minified 64.7 61 -5.7
gzipped 20.4 19.3 -5.4
dist/browser/headless.esm.js bundled 123.7 116.4 -6
minified 72.5 67.5 -7
gzipped 21.2 20.1 -5.5
coveobot commented 4 years ago

@samisayegh commented on 2020-06-23 21:59

Outdated location: line 14 of package.json

It would be a nice touch to add : between cypress and test. I find it makes it easier to read the command.

coveobot commented 4 years ago

@samisayegh commented on 2020-06-23 22:00

Location: line 56 of packages/atomic/README.md

What does “open” do? When would I choose this command over cypresstest?

coveobot commented 4 years ago

@samisayegh commented on 2020-06-23 22:07

Outdated location: line 1 of packages/atomic/cypress/integration/searchBoxTest.ts

The file name convention we’ve been using for atomic and headless is dash-case (kebab-case) for the name and dot to designate the type of file. In this case, it would be:

search-box.test.ts

Another convention I commonly see for functional tests is to use e2e as the type, but this is less important here since the tests live in their own directory rather than next to the component they test.

search-box.e2e.ts

‌

coveobot commented 4 years ago

@samisayegh commented on 2020-06-23 22:09

Location: line 1 of packages/atomic/cypress/utils/network.ts

It’s possible to convert these two functions to use async/await.

coveobot commented 4 years ago

@samisayegh commented on 2020-06-23 22:10

Location: line 1 of packages/atomic/cypress/utils/setupComponent.ts

Same comment on async/await.

coveobot commented 4 years ago

@samisayegh commented on 2020-06-23 22:13

Location: line 14 of packages/atomic/src/pages/test.html

Why is this file needed if we are able to define the markup in the test file? Earlier I saw:

const htmlCode = '<atomic-search-box></atomic-search-box><atomic-result-list></atomic-result-list>';

‌

coveobot commented 4 years ago

@samisayegh commented on 2020-06-23 22:26

Outdated location: line 67 of packages/atomic/cypress/integration/searchBoxTest.ts

I suggestion the following description here: executing a search should log UA.

coveobot commented 4 years ago

@samisayegh approved :heavy_check_mark: the pull request on 2020-06-23 22:26

coveobot commented 4 years ago

Bitbucket user Lucille Vu commented on 2020-06-25 15:16

Location: line 56 of packages/atomic/README.md

that’s for local debug/run test in local, where you will open a cypress ‘local’ dashboard, and choose the test you want to run

coveobot commented 4 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-06-25 16:26

Bundle Size Report

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 131.6 131.6 0
minified 66.5 66.5 0
gzipped 20.9 20.9 0
dist/browser/headless.esm.js bundled 127.6 127.6 0
minified 74.7 74.7 0
gzipped 21.8 21.8 0
coveobot commented 4 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-06-25 17:36

Bundle Size Report

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 131.6 132.8 1
minified 66.5 67.2 1
gzipped 20.9 21 0.8
dist/browser/headless.esm.js bundled 127.6 128.8 0.9
minified 74.7 75.4 0.9
gzipped 21.8 21.9 0.8
coveobot commented 4 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-06-25 20:38

Bundle Size Report

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 132.8 132.8 0
minified 67.2 67.2 0
gzipped 21 21 0
dist/browser/headless.esm.js bundled 128.8 128.8 0
minified 75.4 75.4 0
gzipped 21.9 21.9 0