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.

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

vjsstandard - Tests - Bottom half of test/unit #3477

Merged
merged 3 commits into from
Aug 3, 2016
Merged

vjsstandard - Tests - Bottom half of test/unit #3477

merged 3 commits into from
Aug 3, 2016

Conversation

brandonocasey
Copy link
Contributor

Description

lint the bottom half of the files in the test/unit directory with videojs-standard

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@gkatsev gkatsev added this to the Linting milestone Jul 29, 2016
}
}
});

player.dispose();
});

test('Plugin should have the option of being initilized outside of player init', function(){
expect(3);
QUnit.test('Plugin should have the option of being initilized ' +
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's possible to tell eslint to ignore the long lines rule for tests.

Copy link
Member

Choose a reason for hiding this comment

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

It's just a warning; so, I've been ignoring them.

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 its just a warning and I could put it back to the full line if we want

Copy link
Contributor Author

@brandonocasey brandonocasey Jul 29, 2016

Choose a reason for hiding this comment

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

we could use "ignorePattern": /.*test(.* on the max-len rule or some similar regex. to turn it off for test names, or maybe test names should never be this long?

@brandonocasey brandonocasey changed the title linting bottom half of files in the test/unit dir vjsstandard - Tests - Bottom half of test/unit Jul 29, 2016
move loop iterator into for loop
@brandonocasey
Copy link
Contributor Author

re-commenting on the QUnit.test( max-len issues, just so it does not get lost to the void:

we could use "ignorePattern": /.test(. on the max-len rule or some similar regex. to turn it off for test names, or maybe test names should never be this long?

@misteroneill
Copy link
Member

In other projects, I'm using tsmlj for long strings like this:

QUnit.test(tsmlj`
  this is a really long test name. it has a whole bunch of characters - even more
  than 80 or the warning limit of 90!
`, function(assert) {
  // ...
});

@misteroneill
Copy link
Member

LGTM

@misteroneill misteroneill added the needs: LGTM Needs one or more additional approvals label Aug 2, 2016
import {IE_VERSION} from '../../src/js/utils/browser';
import registerPlugin from '../../src/js/plugins.js';
import Player from '../../src/js/player.js';
import TestHelpers from './test-helpers.js';
import window from 'global/window';
import sinon from 'sinon';
Copy link
Member

Choose a reason for hiding this comment

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

if we're importing sinon directly, we should remove it from the global-shim.js file.

@gkatsev
Copy link
Member

gkatsev commented Aug 2, 2016

A couple of minor comments.

@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Aug 2, 2016
@misteroneill misteroneill merged this pull request into videojs:vjsstandard-core Aug 3, 2016
misteroneill pushed a commit to misteroneill/video.js that referenced this pull request Aug 3, 2016
@brandonocasey brandonocasey deleted the chore/test-linting-pt1 branch August 4, 2016 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants