Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Feb 7, 2020

Converter tests don't need browser to run. Remove browser brings down the test from ~8min to ~5min.


This change is Reviewable

@lina128 lina128 changed the title Remove use of browserstack [WIP] Remove use of browserstack Feb 12, 2020
@lina128 lina128 changed the title [WIP] Remove use of browserstack [converter] Remove use of browserstack Feb 19, 2020
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)


tfjs-converter/package.json, line 61 at r1 (raw file):

    "link-local": "yalc link",
    "publish-local": "yarn build-npm && yalc push",
    "test": "yarn gen-json --test && karma start",

no more karma for local too right? let's make yarn test call node and call yarn test from test-ci


tfjs-converter/package.json, line 62 at r1 (raw file):

    "publish-local": "yarn build-npm && yalc push",
    "test": "yarn gen-json --test && karma start",
    "test-ci": "yarn gen-json --test && yarn build && yarn lint && ts-node run_tests.ts",

can you update cloudbuild.yml there is a line called "test-browser" you can rename it


tfjs-converter/package.json, line 64 at r1 (raw file):

    "test-ci": "yarn gen-json --test && yarn build && yarn lint && ts-node run_tests.ts",
    "test-snippets": "ts-node ./scripts/test_snippets.ts",
    "run-browserstack": "karma start --singleRun --browsers='bs_firefox_mac,bs_chrome_mac' --reporters='dots,karma-typescript,BrowserStack'",

remove this

@nsthorat
Copy link
Contributor

Can you also make sure coverage hasn't decreased by running master against the CPU backend and make sure its the same number (looks like 470: https://pantheon.corp.google.com/cloud-build/builds/b3e40bd6-7734-4b76-98f8-576f3a2a3bb9;step=2?project=learnjs-174218)

@lina128
Copy link
Collaborator Author

lina128 commented Feb 20, 2020

Hi @nsthorat , thank you for your review! I cleaned up cloudbuild and package.json. Can you re-review the PR? And yes, the converage is the same.

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @lina128, and @pyu10055)


tfjs-converter/cloudbuild.yml, line 62 at r3 (raw file):

  entrypoint: 'bash'
  args: ['./run-python-tests.sh']
  waitFor: ['python-build']

how come do we have to wait for python-build now?

@lina128
Copy link
Collaborator Author

lina128 commented Feb 20, 2020

Reviewed 2 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @lina128, and @pyu10055)

tfjs-converter/cloudbuild.yml, line 62 at r3 (raw file):

  entrypoint: 'bash'
  args: ['./run-python-tests.sh']
  waitFor: ['python-build']

how come do we have to wait for python-build now?

Ah, you are right, I thought the previous step is to build package, it's actually just testing package, so they can run in parallel. Changed back to before.

@lina128 lina128 merged commit fe39e34 into tensorflow:master Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants