Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Update the test runner config to support async/await in tests. #1068

Merged
merged 3 commits into from
Jun 1, 2018

Conversation

tafsiri
Copy link
Contributor

@tafsiri tafsiri commented May 30, 2018

Description

Allows use of async/await in tests. I updated one test to show what that would look like. Admittedly it makes that test inconsistent with the other tests if the file so I can drop that if preferred.


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

@tafsiri tafsiri requested review from dsmilkov and caisq May 30, 2018 20:22
@dsmilkov
Copy link
Contributor

dsmilkov commented Jun 1, 2018

Review status: 2 of 5 files reviewed at latest revision, all discussions resolved, some commit checks failed.


karma.conf.js, line 27 at r1 (raw file):

    karmaTypescriptConfig: {
      tsconfig: 'tsconfig-es2017.json',
      compilerOptions: {module: 'commonjs', sourceMap: true}

do you even need a separate tsconfig-es2017 file, or you can just pass in the "target": "es2017" in the compilerOptions here?


package.json, line 22 at r1 (raw file):

    "@types/seedrandom": "~2.4.27",
    "clang-format": "~1.2.2",
    "jasmine-core": "~2.99",

curious why not the latest, jasmine 3.1.0: https://www.npmjs.com/package/jasmine-core


src/io/local_storage_test.ts, line 160 at r1 (raw file):

  it('Save-load round trip succeeds', async () => {
    const handler1 = tf.io.getSaveHandlers('localstorage://FooModel')[0];
    try {

do you need the try/catch at all? Hopefully the error just propagates and surfaces to the user


Comments from Reviewable

@tafsiri
Copy link
Contributor Author

tafsiri commented Jun 1, 2018

The final update to this after investigation is that the only thing we need to do is update jasmine. Most likely the typescript compiler output es5 output has improved support for transpiling async/await that wasn't present the last time this wasn't investigated.


Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


karma.conf.js, line 27 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

do you even need a separate tsconfig-es2017 file, or you can just pass in the "target": "es2017" in the compilerOptions here?

Done


package.json, line 22 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

curious why not the latest, jasmine 3.1.0: https://www.npmjs.com/package/jasmine-core

was initially to catch any deprecations between major versions. Updating.


src/io/local_storage_test.ts, line 160 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

do you need the try/catch at all? Hopefully the error just propagates and surfaces to the user

Done


Comments from Reviewable

@dsmilkov
Copy link
Contributor

dsmilkov commented Jun 1, 2018

:lgtm_strong:


Review status: 3 of 4 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@dsmilkov dsmilkov merged commit d001ce5 into master Jun 1, 2018
@dsmilkov dsmilkov deleted the update-test-runner branch June 1, 2018 17:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants