Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Mar 24, 2020

This PR has below changes:

  1. Remove temporary solution because symlink won't be changed by release any more.
  2. Add wasm to integration test.
  3. Refactor package.json and cloudbuild.yml to be be consistent with other packages.
  4. Add dependency to package_dependencies.json.

This change is Reviewable

@lina128 lina128 changed the title Refactor [wasm] Test against head. Mar 24, 2020
@lina128 lina128 requested review from dsmilkov and nsthorat and removed request for dsmilkov March 24, 2020 05:10
@lina128 lina128 requested review from dsmilkov and pyu10055 March 24, 2020 05:16
@nsthorat nsthorat requested a review from tafsiri March 24, 2020 13:32
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 6 of 6 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, @pyu10055, and @tafsiri)


tfjs-backend-wasm/package.json, line 24 at r1 (raw file):

    "cpplint": "./scripts/cpplint.js",
    "lint": "tslint -p . -t verbose && yarn cpplint",
    "test": "yarn && yarn build-deps && yarn build && karma start",

one thing i realized, this means every time you run test you have to build all the dependency projects. this can slow things down -- is there any way to split these up (for all packages)? fine to submit as is for now, just something to think about.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

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


tfjs-backend-wasm/package.json, line 24 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

one thing i realized, this means every time you run test you have to build all the dependency projects. this can slow things down -- is there any way to split these up (for all packages)? fine to submit as is for now, just something to think about.

maybe we can add "test-dev" or "test-fast" across all repos, used for development. In general I like that "yarn test" is doing all the things since now that everything is at master, we will likely touch several files across packages in the same commit/PR.

Copy link
Collaborator Author

@lina128 lina128 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: :shipit: complete! 2 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, @pyu10055, and @tafsiri)


tfjs-backend-wasm/package.json, line 24 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

maybe we can add "test-dev" or "test-fast" across all repos, used for development. In general I like that "yarn test" is doing all the things since now that everything is at master, we will likely touch several files across packages in the same commit/PR.

Thank you Nikhil and Daniel! Great suggestion. I can add a test-fast command for all repos in a separate PR.

@lina128 lina128 merged commit bc228dc into tensorflow:master Mar 24, 2020
@lina128 lina128 deleted the refactor branch March 24, 2020 14:22
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.

4 participants