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

feat: component testing #14479

Merged
merged 511 commits into from
Feb 4, 2021
Merged

feat: component testing #14479

merged 511 commits into from
Feb 4, 2021

Conversation

brian-mann
Copy link
Member

No description provided.

brian-mann and others added 30 commits January 5, 2021 02:27
- TODO: need to fix our custom circular parser to work with socket.io
engine 3.x.x
…ocket-ct

- move all reusable logic into socket-base, only split out what is
different in each socket class
* types: fix types

* types: use target es6

* types: use Promise for async/await

* types: lint
- add middleware interfaces
- provide good error reason as to why the property isn’t yet available
- use TS generics for proper type autocompletion
Copy link
Contributor

@kuceb kuceb left a comment

Choose a reason for hiding this comment

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

cant comment on the files individually
npm/vue/examples/cli-ts/cypress/plugins/index.ts is a .ts file but uses requires, any reason?
looks like this PR adds back in no-only eslint plugin, which we removed previously. possibly a mis-merge? I can push a commit to fix.

@kuceb
Copy link
Contributor

kuceb commented Feb 3, 2021

enabled running example repos and testing binary build for this branch

flotwig
flotwig previously approved these changes Feb 3, 2021
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

This looks fine for initial beta release - my comments are all things that can be fixed in future PRs.

My only major concerns are:

  • code duplication - in the server-ct, and especially in the runner-ct, lots of code is duplicated. it would be a shame if these unnecessarily diverged and introduced inconsistent user experience, so i feel that a refactor of both these areas is in immediate need
  • this is not specific to CT, but the type exports (cli/types/...) are a messssss and are only getting worse :/

Super happy about all the typescriptification in this PR too, and excited for CT to drop!

this._urlResolver = null
}

open (config = {}, project, onError, onWarning) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be deduplicated with server-ct#open, it is vastly the same

.eslintrc.json Show resolved Hide resolved
Comment on lines +2 to +3
// See https://go.microsoft.com/fwlink/?LinkId=733558
// for the documentation about the tasks.json format
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, i wonder if we should convert our terminals.json to this format

Copy link
Contributor

Choose a reason for hiding this comment

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

no idea - I don't use these at all, I guess someone else aded them. Might check it out and so I can start using vscode more effectively...

Comment on lines +34 to +41
{
"name": "packages/server test-e2e",
"focus": true,
"onlySingle": true,
"execute": false,
"cwd": "[cwd]/packages/server",
"command": "yarn test-e2e [fileBasename]"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

should work the same as packages/server test, so doesn't really need to be added

Comment on lines +648 to +661
server-ct-unit-tests:
<<: *defaults
parallelism: 1
steps:
- attach_workspace:
at: ~/
- check-conditional-ci
- run: yarn test-unit --scope @packages/server-ct
- verify-mocha-results:
expectedResultCount: 1
- store_test_results:
path: /tmp/cypress
- store-npm-logs

Copy link
Contributor

Choose a reason for hiding this comment

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

so, unfortunately, we are over github's 100 checks/commit limit in develop...... this makes it urgent to find a way to cut down, as with every new job we add, the chance of another check randomly failing and going unreported increases

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Oof. We're gonna need to get on the phone and figure out how to do that.

Comment on lines +242 to +243

return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return

lolwut

@@ -236,6 +236,12 @@ export = {

debug('found browsers %o', { browsers })

if (!process.versions.electron) {
debug('not in electron, skipping adding electron browser')
Copy link
Contributor

Choose a reason for hiding this comment

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

so happy to see us start doing node-only testing, users have been asking for this

return require('./smoke_test').run(options)
default:
break
if (mode === 'record') {
Copy link
Contributor

Choose a reason for hiding this comment

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

aw i liked the switch statement

options.url = url

options.isTextTerminal = cfg.isTextTerminal
_.defaults(options, {
Copy link
Contributor

Choose a reason for hiding this comment

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

good place to use pick

if (inspector.url()) {
childOptions.execArgv = _.chain(process.execArgv.slice(0))
.remove('--inspect-brk')
.push(`--inspect=${process.debugPort + 1}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess this kinda addresses #5802 ?

kuceb
kuceb previously approved these changes Feb 3, 2021
@JessicaSachs JessicaSachs dismissed stale reviews from kuceb and flotwig via ad08b6d February 3, 2021 23:02
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.

None yet

7 participants