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

Introducing E2E Tests #45

Merged
merged 2 commits into from
Aug 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 35 additions & 3 deletions .github/workflows/node.js.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,14 @@ jobs:

strategy:
matrix:
node-version: [10.x, 12.x, 14.x]
node-version: [12.x]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NMinhNguyen I didn't realise previously that we were resource constrained. For now I'm going to restrict what we use until we have a need.


steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}

- name: Get Yarn cache directory path
id: yarn-cache-dir-path
run: echo "::set-output name=dir::$(yarn cache dir)"
Expand All @@ -62,9 +61,42 @@ jobs:
key: v2-${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
v2-${{ runner.os }}-yarn-

- run: yarn
- run: yarn test
- name: Install examples dependencies
if: steps.yarn-cache.outputs.cache-hit != 'true'
run: yarn --cwd examples/app-host install
test_e2e:
runs-on: ubuntu-latest

strategy:
matrix:
node-version: [12.x]

steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}
- name: Get Yarn cache directory path
id: yarn-cache-dir-path
run: echo "::set-output name=dir::$(yarn cache dir)"
- name: Cache Yarn
uses: actions/cache@v2
id: yarn-cache
with:
path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
key: v2-${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
v2-${{ runner.os }}-yarn-
- name: Install dependencies
run: yarn
env:
PUPPETEER_SKIP_CHROMIUM_DOWNLOAD: "true"
- name: Run E2E tests
uses: ianwalter/puppeteer-container@v4.0.0
with:
args: yarn e2e --forceExit
Copy link
Contributor Author

@sebinsua sebinsua Aug 19, 2020

Choose a reason for hiding this comment

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

Sorry, about the --forceExit.

I tried so many different ways of killing processes (e.g. devServerProcess.kill('SIGKILL'), fkill(devServerProcess.pid, { force: true}), as well as using port-pid to find pids with the specified open port and killing those).

It turned out there were no pids returned by port-pid but port 3000 was in use anyway. I'm wondering if there is some kind of overlap or race condition with how tests run but that would be even more of a rabbit hole to debug.

I'll add a comment to try to debug further, linking to this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

does CI=true help?

(It should already be set by default, but just verifying)

Copy link
Contributor Author

@sebinsua sebinsua Aug 19, 2020

Choose a reason for hiding this comment

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

It didn't help when locally running with Docker.

Yeah, it should be set by default on CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

--forceExit is totally fine for now (assuming it doesn't swallow errors), feel free to file a followup task; sounds like a good issue for an external contributor.

env:
CI: "true"
41 changes: 41 additions & 0 deletions e2e-tests/TestApp.test-tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// We need to ensure that the `app/src/App.tsx` used by `yarn start` renders widgets
// in order that we can test that an application can render widgets.
// Therefore, this is a copy of `App.tsx` which renders all of the widgets.
// I've changed the extension, because I don't want it to be picked up by TypeScript or ESLint.
import * as React from 'react';
sebinsua marked this conversation as resolved.
Show resolved Hide resolved
import logo from './logo.svg';
import './App.css';

import widgets from './widgets';

function App(): JSX.Element {
return (
<div className="App">
<header className="App-header">
<img src={logo} className="App-logo" alt="logo" />
<p>
Edit <code>src/App.tsx</code> and save to reload.
</p>
<a
className="App-link"
href="https://reactjs.org"
target="_blank"
rel="noopener noreferrer"
>
Learn React
</a>
</header>
<div data-testid="widgets">
<React.Suspense fallback={null}>
{Object.keys(widgets).length > 0
? Object.entries(widgets).map(([widgetName, Widget]) => (
<Widget key={widgetName} />
))
: 'No widgets found...'}
</React.Suspense>
</div>
</div>
);
}

export default App;