Skip to content

Conversation

@mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Jun 10, 2022

Update jasmine from 3.1.0 to 4.2.1 for Bazel packages. Update karma from 6.3.16 to 6.4.0.

Remove deprecated use of async function and done callback at the same time. For example, the following code

it('does a test', async (done) => {
  if(!okay()) {
    done.fail();
  }
  done();
});

becomes this instead.

it('does a test', async () => {
  if(!okay()) {
    fail();
  }
});

Add karma-jasmine-html-reporter to show test status in the browser window.

Depends on #6563

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

mattsoulanille and others added 17 commits June 9, 2022 17:28
According to the ESModules standard, code can not modify properties of modules. TypeScript 4 will enforce this rule. This in particular affects tests for executors in tfjs-converter, in which we often spy on tfOps.

This PR removes all instances of `spyOn(tfOps,...)` and replaces them with a separate `spyOps` mock / fake which is passed to the `executeOp` function.

This PR was part of the larger TS4 upgrade PR (tensorflow#6346), but I'm splitting that PR into pieces that can be merged while we're still using TS3 because it's too large to keep up-to-date.
@mattsoulanille mattsoulanille changed the title Display Jasmine test results in Karma browsers Update karma and jasmine Jun 24, 2022
@mattsoulanille mattsoulanille changed the title Update karma and jasmine Update Jasmine to 4.2.1 Jun 24, 2022
@mattsoulanille mattsoulanille changed the title Update Jasmine to 4.2.1 Update Jasmine from 3.1.0 to 4.2.1 Jun 24, 2022
@mattsoulanille mattsoulanille marked this pull request as ready for review June 29, 2022 00:06
Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

LGTM!

coverageOptions: {instrumentation: false},
bundlerOptions: {
sourceMap: true,
acornOptions: {ecmaVersion: 8},
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a typescript compilation issue in e2e tests.

"@tensorflow/tfjs-backend-webgl": "^3.9.0",
"@types/jasmine": "~2.8.6",
"clang-format": "~1.8.0",
"jasmine": "3.1.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

jasmine was missing as a dependency of tfjs-automl, so it was using the root jasmine version. This caused tests to fail when the root jasmine was upgraded to 4.

"bs_chrome_mac",
"win_10_chrome",
],
local_browser = "chrome_autoplay",
Copy link
Member Author

Choose a reason for hiding this comment

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

Use chrome_autoplay custom browser so media can play without user interaction.

const videoElement = document.createElement('video');
videoElement.width = 160;
videoElement.height = 90;
describeWithFlags('readers in browser', MEDIA_ENVS, () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

MEDIA_ENVS is a new constraint set that only runs if navigator.mediaDevices is not null. It's required because jasmine 4 prohibits empty describe blocks, which the if statement would cause.


export const MEDIA_ENVS: Constraints = {
predicate: (env) => BROWSER_ENVS.predicate(env)
&& navigator.mediaDevices != null
Copy link
Member Author

Choose a reason for hiding this comment

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

New Constraints set to only run if media devices are available.

Comment on lines +73 to +76
chrome_autoplay: {
base: 'Chrome',
flags: ['--autoplay-policy=no-user-gesture-required'],
},
Copy link
Member Author

Choose a reason for hiding this comment

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

The chrome_autoplay browser allows media to play without user interaction.

}

config.set({
reporters: ['kjhtml'],
Copy link
Member Author

Choose a reason for hiding this comment

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

New in-browser karma reporter.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

thanks!

Reviewed 34 of 34 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @mattsoulanille)


tfjs-automl/package.json line 35 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

jasmine was missing as a dependency of tfjs-automl, so it was using the root jasmine version. This caused tests to fail when the root jasmine was upgraded to 4.

awesome!


tfjs-backend-wasm/src/index_test.ts line 240 at r1 (raw file):

// describe('wasm pre.js', () => {
  // Temporarily disabled due to node 16 incompatability

this test is commented out?


tools/karma_template.conf.js line 76 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

The chrome_autoplay browser allows media to play without user interaction.

how is the purpose for using this flag?


tools/karma_template.conf.js line 127 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

New in-browser karma reporter.

nice!

@mattsoulanille mattsoulanille merged commit 189ac32 into tensorflow:master Jun 29, 2022
@mattsoulanille mattsoulanille requested a review from pyu10055 June 29, 2022 23:28
Copy link
Member Author

@mattsoulanille mattsoulanille 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 @mattsoulanille and @pyu10055)


tfjs-backend-wasm/src/index_test.ts line 240 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

this test is commented out?

I should maybe remove this test. It requires removing the process variable, which isn't really supported in node 16+.


tools/karma_template.conf.js line 76 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

how is the purpose for using this flag?

It's required for some tfjs-data tests that play media. We need the media to play in karma tests without human interaction.

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.

3 participants