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.

Sign up for GitHub

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

Jump to bottom

test: cleanup test warnings #4752

Merged
merged 3 commits into from
Nov 16, 2017
Merged

test: cleanup test warnings #4752

merged 3 commits into from
Nov 16, 2017

Conversation

brandonocasey
Copy link
Contributor

Description

Currently we get logs in the tests due to a recent change. This PR fixes that. It also fixes a case were we would log that the dom element isn't in the dom, when it really is:

Here is an example (taken from the test that showed me it)

const fixture = document.getElementById('qunit-fixture');

fixture.innerHTML += '<video id="test_vid_id"></video>';

const tag = document.getElementById('test_vid_id');

const player = videojs(tag)

// this would log a warning about tag not being in the dom. Technically its not because
// of the changes we made to it. I would even say that this is a bad way to get the player again.
// but I think that warning that the players tag is not in the dom on the first creation is enough, and // warning that it isn't in the dom because the tag has been changed is a bit weird.

// returns the same player as above.
playerAgain(tag);

gkatsev
gkatsev reviewed Nov 16, 2017
View reviewed changes
src/js/video.js Outdated
// Element may have a player attr referring to an already created player instance.
// If so return that otherwise set up a new player below
if (tag.player || Player.players[tag.playerId]) {
return tag.player || Player.players[tag.playerId];
}

// Check if element is included in the DOM
Copy link
Member

Choose a reason for hiding this comment

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

this change would make this be fix rather than a test

src/js/video.js Outdated
// Element may have a player attr referring to an already created player instance.
// If so return that otherwise set up a new player below
if (tag.player || Player.players[tag.playerId]) {
return tag.player || Player.players[tag.playerId];
}

// Check if element is included in the DOM
Copy link
Member

Choose a reason for hiding this comment

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

should we have this be a separate PR from the test changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure

test/unit/player.test.js Outdated
}, 'a falsey techOrder should throw');

videojs.options.techOrder = techOrder;

while (fixture.firstChild) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to do this. qunit should be taking care to empty out qunit-fixture element after a test is run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

@brandonocasey
Copy link
Contributor Author

See #4755 for the other PR

@gkatsev gkatsev merged commit 3aae4b2 into master Nov 16, 2017
@gkatsev gkatsev deleted the test/cleanup-logs branch November 16, 2017 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkatsev gkatsev gkatsev approved these changes

@ldayananda ldayananda ldayananda approved these changes

Assignees
No one assigned
Labels
confirmed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brandonocasey @gkatsev @ldayananda