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

e2e tests #346

Merged
merged 4 commits into from
May 31, 2018
Merged

e2e tests #346

merged 4 commits into from
May 31, 2018

Conversation

mathieudutour
Copy link
Collaborator

@mathieudutour mathieudutour commented May 30, 2018

  • update to babel 7
  • add a warning when trying to render something that is not a Document in a Document
  • add a warning when trying to render a page in something that is not a page
  • add a basic e2e test

To run the E2E tests: npm run test:e2e. That will start sketch and run the tests which are under __tests__/skpm/

There might be some changes in the generated files due to updating to babel 7, I haven't checked yet

Copy link
Collaborator

@jaridmargolin jaridmargolin left a comment

Choose a reason for hiding this comment

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

A few changes looped in here that I am not sure are necessary.

Additionally, is anything stopping us from running e2e tests on Travis? https://github.com/skpm/skpm/tree/master/packages/test-runner#running-the-tests-on-travisci

"presets": ["module:@skpm/babel-preset", "@babel/preset-flow"],
"plugins": ["@babel/plugin-proposal-class-properties", ["@babel/plugin-transform-modules-commonjs", {
"noInterop": true
}]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this required by skpm?

["@babel/plugin-transform-modules-commonjs", { "noInterop": true }]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The plugin is required to export commonjs modules. It’s not the default with Babel 7 anymore. I chose to add the noInterop option because it reduces the bundle size a lot. But it also means that I needed to change a bunch of imports in order to make them compatible. For example react doesn’t export a default so you need to import * as react from it. This is actually the correct way to import a commonjs package (typescript enforces it).

Copy link
Collaborator

@jaridmargolin jaridmargolin May 30, 2018

Choose a reason for hiding this comment

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

I had no idea that React doesn't ship an es module distro 🙀

I suppose this all makes sense internally.

Externally, should we be updating our docs and examples? While it may be correct, wonder if it may cause confusion. And maybe I am incorrect here, but I would expect most people are used to seeing import React from 'react'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah happy to change the docs back

@@ -37,7 +37,7 @@ Managing the assets of design systems in Sketch is complex, error-prone and time
## What does the code look like?

```js
import React from 'react';
import * as React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reasoning for altering all of these imports?


const page = sketch.Page.fromNative(nativePage);
expect(page.layers[0].name).toBe('Swatches');
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an absolutely awesome start 🙌

@mathieudutour
Copy link
Collaborator Author

Nothing stopping us from running it on Travis no, just didn’t have time to set it up yet.
Also I’m not sure I have access to the setting to set a sketch license in the env variables

@jaridmargolin
Copy link
Collaborator

I'd love to see this run on CI. Can be done in a standalone PR if you prefer...

@mathieudutour mathieudutour merged commit 466ac54 into master May 31, 2018
@mathieudutour mathieudutour deleted the feature/e2e-tests branch May 31, 2018 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants