Skip to content
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

add testing and refactor stories to work with it #63

Merged
merged 6 commits into from
Jul 26, 2021
Merged

Conversation

mabry1985
Copy link
Contributor

@mabry1985 mabry1985 commented Jul 18, 2021

Testing steps

Testing

  • Remove node_modules
  • Run yarn install
  • Run yarn test
  • ...?

Storybook

  • Run yarn start
  • Confirm stories are still rendering as expected. (you don't get an error when viewing components).

Add a known failing test (optional)

Known failing test can be added in src/components/base/outline-button/test/outline-button.test.ts

describe('failing test', () => {
  it('expects to fail', () => {
    assert.equal(1, 2, "1 is not 2")
  });
});

Then run yarn test again and see the failure notice.

@mabry1985 mabry1985 requested a review from himerus July 18, 2021 22:01
@mabry1985 mabry1985 self-assigned this Jul 18, 2021
@@ -1,4 +1,4 @@
import { html, HTMLTemplateResult } from 'lit';
import { html, TemplateResult } from 'lit-html';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a difference between the html in lit and lit-html and a reason for picking one or the other?

I noticed in one of the components that lit-html doesn't have CSSResultGroup, but lit does. outline-button uses this as an example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are exclusively supposed to be using the lit package only now, however, this seems like a work around for this issue with what was going on with stories no longer rendering. @mabry1985 did you have a link to an issue in Lit (or openwc/testing) related to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So previously the stories were breaking when we installed the @open-web/testing dependency and pulling html straight from lit-html instead of lit fixed the issue. Which is strange because if you look at the html import from lit it seems to be just passing through html from lit-html

We would still exclusively use the imports from lit when building the components themselves though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@himerus I'm currently in discussion with someone on the Lit slack about seeing if we can sort this out I'll open an issue if that doesn't get matters resolved. I think the workaround is okay for now though.

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@damontgomery damontgomery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I was able to get this to run as expected. But, please let me know if I can help by reviewing something else too.

Questions

  • The yarn.lock file is changed after I run yarn install can we review that?

Testing steps

  • deleted node_modules
  • Installed packages with yarn install
  • run yarn test
  • Confirm everything passes
  • Add a known failing test (see below)
  • run yarn test
  • Confirm one test fails

Known failing test added in /Users/dan/Sites/outline/src/components/base/outline-button/test/outline-button.test.ts

describe('failing test', () => {
  it('expects to fail', () => {
    assert.equal(1,2,"1 is not 2")
  })
})

Results

: yarn test
yarn run v1.22.10
$ tsc && wtr

Chromium: |██████████████████████████████| 9/9 test files | 25 passed, 0 failed
Firefox:  |██████████████████████████████| 9/9 test files | 25 passed, 0 failed
Webkit:   |██████████████████████████████| 9/9 test files | 25 passed, 0 failed

Finished running tests in 13.1s, all tests passed! 🎉

✨  Done in 22.91s.
: yarn test
yarn run v1.22.10
$ tsc && wtr

dist/components/base/outline-button/test/outline-button.test.js:

 ❌ failing test > expects to fail
      AssertionError: 1 is not 2: expected 1 to equal 2
      + expected - actual

      -1
      +2

      at o.<anonymous> (src/components/base/outline-button/test/outline-button.test.ts:6:11)

Chromium: |██████████████████████████████| 9/9 test files | 25 passed, 1 failed
Firefox:  |██████████████████████████████| 9/9 test files | 25 passed, 1 failed
Webkit:   |██████████████████████████████| 9/9 test files | 25 passed, 1 failed

Finished running tests in 8.3s with 3 failed tests.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
:

package.json Outdated
@@ -59,7 +59,8 @@
"reboot": "yarn clean && yarn reset && yarn install && yarn start",
"rollup.bundle": "rollup -c -m",
"rollup.watch": "yarn rollup -w",
"rollup": "yarn rollup.bundle"
"rollup": "yarn rollup.bundle",
"test": "tsc && wtr"
Copy link
Contributor

@damontgomery damontgomery Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I haven't run storybook (or compiled the stylesheets), then the TypeScript compiler complains about the references to non-existing files.

src/components/base/outline-accordion/outline-accordion.ts:3:29 - error TS2307: Cannot find module './outline-accordion.css.lit' or its corresponding type declarations.

3 import componentStyles from './outline-accordion.css.lit';

Should we add generate.component-css to this task?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running yarn build instead of tsc in this test script will solve the above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mabry1985 If we change the above yarn test command as described, I think we may be good to go here.

Copy link
Contributor

@damontgomery damontgomery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me!

I left a comment about the compiled styles for the yarn command.

Still also getting changes in yarn.lock, but I don't know what to make of that.

Storybook works well and shows the elements. It also doesn't have issues with empty attributes as it did at one point. Those would show up as Symbol(lit-nothing).

package.json Outdated Show resolved Hide resolved
package.json Outdated
@@ -59,7 +59,8 @@
"reboot": "yarn clean && yarn reset && yarn install && yarn start",
"rollup.bundle": "rollup -c -m",
"rollup.watch": "yarn rollup -w",
"rollup": "yarn rollup.bundle"
"rollup": "yarn rollup.bundle",
"test": "tsc && wtr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running yarn build instead of tsc in this test script will solve the above.

package.json Outdated
@@ -59,7 +59,8 @@
"reboot": "yarn clean && yarn reset && yarn install && yarn start",
"rollup.bundle": "rollup -c -m",
"rollup.watch": "yarn rollup -w",
"rollup": "yarn rollup.bundle"
"rollup": "yarn rollup.bundle",
"test": "tsc && wtr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mabry1985 If we change the above yarn test command as described, I think we may be good to go here.

@himerus
Copy link
Contributor

himerus commented Jul 26, 2021

After the last commit, including a new yarn.lock, this seems to install cleanly with a yarn reboot.

@himerus himerus merged commit aa9f07f into next Jul 26, 2021
@himerus himerus deleted the feature/testing branch July 26, 2021 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants