Skip to content

New tests#192

Merged
miripiruni merged 1 commit intomasterfrom
new-tests
Feb 5, 2016
Merged

New tests#192
miripiruni merged 1 commit intomasterfrom
new-tests

Conversation

@miripiruni
Copy link
Contributor

@miripiruni miripiruni commented Feb 2, 2016

v4.x branch: 106 tests. This branch: 291 tests.

  • All old tests was reorganized in structure.
  • More tests added.
  • mocha updated to 2.4.5
  • All bugs from this tests have label From BH tests.
  • npm run lint now run JSHint on ./test/*.js

As bonus:

  • You can run tests separately.
  • Error in the fall tests become more understandable (there is a link to a file).

@miripiruni miripiruni mentioned this pull request Feb 2, 2016
Closed
@tadatuta
Copy link
Member

tadatuta commented Feb 2, 2016

lgtm

@qfox
Copy link
Member

qfox commented Feb 2, 2016

lgtm too

@miripiruni
Copy link
Contributor Author

miripiruni commented Feb 3, 2016

May I merge?

@qfox
Copy link
Member

qfox commented Feb 3, 2016

/cc @indutny

@indutny
Copy link
Contributor

indutny commented Feb 3, 2016

I have several concerns about this PR:

  1. (nit) uppercase letters in path are kind of lame, I don't see any value in having them here, except that it makes tab completion a bit harder on linux (Let's punish those linux guys!)
  2. Having asserts-test.js is against the test logic. These asserts are regression tests for the functionality that is tested in nearby functions. If you group them together and put in one file, it won't really help anyone, in my opinion.
  3. applyNext/applyCtx/localdoes not really belong to BEMContext, I guess? It is more likely a part of runtime

@miripiruni miripiruni force-pushed the new-tests branch 3 times, most recently from 62e57cd to 9f3996b Compare February 4, 2016 15:00
@miripiruni
Copy link
Contributor Author

miripiruni commented Feb 4, 2016

@indutny thank you.

  1. My bad. Fixed.
  2. I agree with you. Fixed.
  3. Agree. Fixed.

@miripiruni miripiruni force-pushed the new-tests branch 3 times, most recently from 276e89b to 4af8272 Compare February 4, 2016 16:22
indutny
indutny reviewed Feb 4, 2016
View reviewed changes
test/bemhtml-test.js Outdated
var fixtures = require('./fixtures')('bemhtml');
var test = fixtures.test;

describe('BEMHTML compiler/Tree', function() {
Copy link
Contributor

@indutny indutny Feb 4, 2016

Choose a reason for hiding this comment

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

Tree?

@indutny
Copy link
Contributor

indutny commented Feb 4, 2016

LGTM

@miripiruni miripiruni force-pushed the new-tests branch 2 times, most recently from 64bde3a to 3d8eac3 Compare February 5, 2016 11:34
miripiruni added a commit that referenced this pull request Feb 5, 2016
@miripiruni
Merge pull request #192 from bem/new-tests
5576225 
New tests
@miripiruni miripiruni merged commit 5576225 into master Feb 5, 2016
@miripiruni miripiruni deleted the new-tests branch February 5, 2016 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@miripiruni @tadatuta @qfox @indutny