Skip to content

Conversation

mgol
Copy link
Member

@mgol mgol commented Mar 3, 2016

Fixes #152

I tried to pay attention to what I'd been removing but I'd apprieciate a 2nd (or 3rd) pair of eyes. :D

The build is failing because of low test coverage (79.65%, below the 85% threshold) as I mainly removed well tested code. I'll add some tests once it's verified I haven't removed too much.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @dmethvin, @markelog and @gibson042 to be potential reviewers

@mgol
Copy link
Member Author

mgol commented Mar 3, 2016

The module is not really Core but lots of modules but that wouldn't even fit in our message limit.

@mgol
Copy link
Member Author

mgol commented Mar 3, 2016

Ah, coverage is fine, I just haven't run grunt build before running npm test. We should get the npm scripts to be equivalent to what we have in Core, current setup is confusing.

@markelog
Copy link
Member

markelog commented Mar 3, 2016

Nice!

I just haven't run grunt build before running npm test.

Hm, npm test should work without build, what are repro steps?

@mgol
Copy link
Member Author

mgol commented Mar 3, 2016

Steps:

  1. Run grunt on master.
  2. Apply my PR.
  3. Run npm test.

@markelog
Copy link
Member

markelog commented Mar 3, 2016

Can't repro -

$ grunt
Running "concat:dist" (concat) task
File dist/jquery-migrate.js created.

Running "uglify:all" (uglify) task
>> 1 file created.

Running "jshint:dist" (jshint) task
>> 1 file lint free.

Running "jshint:tests" (jshint) task
>> 12 files lint free.

Running "jshint:grunt" (jshint) task
>> 1 file lint free.

Running "jscs:src" (jscs) task
>> 27 files without code style errors.

Running "qunit:files" (qunit) task
Testing test/index.html JQMIGRATE: Migrate is installed with logging active, version 1.3.1-pre
...............................................OK
>> 774 assertions passed (1637ms)
>> Coverage:
>> -  Lines: 86.04%
>> -  Statements: 86.04%
>> -  Functions: 90%
>> -  Branches: 70.33%

Done, without errors.
$ g fpr 155
From github.com:jquery/jquery-migrate
 * [new ref]         refs/pull/155/head -> 155
Switched to branch '155'
$ npm test

> jquery-migrate@3.0.0-pre test /Users/arkel/Workspace/jquery-migrate
> grunt test

Running "qunit:files" (qunit) task
Testing test/index.html JQMIGRATE: Migrate is installed with logging active, version 1.3.1-pre
.....................OK
>> 62 assertions passed (1211ms)
>> Coverage:
>> -  Lines: 87.25%
>> -  Statements: 87.25%
>> -  Functions: 88.89%
>> -  Branches: 70.49%

Done, without errors.

"src/attributes.js",
"src/core.js",
"src/css.js",
"src/data.js",
Copy link
Member

Choose a reason for hiding this comment

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

This may be back soon if #87 is fixed, but we might as well take it out now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I may leave it empty if you wish but adding new modules isn't that hard anyway. :)

@mgol
Copy link
Member Author

mgol commented Mar 3, 2016

@markelog OK, the steps I provided don't work but it definitely happened to me. I was working on the patch, removing more & more stuff & running npm test in the meantime (which I sometimes forgot to preceed by grunt build) and that's what I got:

npm test

> jquery-migrate@3.0.0-pre test /Users/mgol/Documents/projects/public/jquery/jquery-migrate
> grunt test

Running "qunit:files" (qunit) task
Testing test/index.html JQMIGRATE: Migrate is installed with logging active, version 1.3.1-pre
.....................OK
>> 62 assertions passed (1187ms)
>> Coverage:
>> -  Lines: 79.65%
>> -  Statements: 79.65%
>> -  Functions: 76.19%
>> -  Branches: 68.25%
Warning: Code coverage for lines was below threshold (79.65% < 85%) Use --force to continue.

Aborted due to warnings.
npm ERR! Test failed.  See above for more details.

Even regardless of this issue I'd still want to see the build script to be prepended by npm install and the test one by grunt build as in https://github.com/jquery/jquery/blob/93a8fa6bfc1c8a469e188630b61e736dfb69e128/package.json#L59-L64. Let's be consistent.

@markelog
Copy link
Member

markelog commented Mar 3, 2016

@mgol Sounds like you were on intermediate step - removed the tests, but not the code, or didn't applied latest migrate patches i.e. before the rebase.

Difference between the migrate and core, that in the latter you have to build (if not running in the amd mode which not default) before run the tests, which is not necessary for the migrate, since migrate, by default, would still not use your builded version.

@mgol
Copy link
Member Author

mgol commented Mar 3, 2016

Difference between the migrate and core, that in the latter you have to build (if not running in the amd mode which not default) before run the tests, which is not necessary for the migrate, since migrate, by default, would still not use your builded version.

Even then, npm test should invoke all tests, including running linters and since both Core & Migrate have additional JSHint setup for the built file, this require building. I'd like to align with Core here.

@markelog
Copy link
Member

markelog commented Mar 3, 2016

Mmm, okay, sounds logical, when you think about that, a lot of projects do the same like in babel-register case for example, plus alignment would also be nice.

@markelog
Copy link
Member

markelog commented Mar 3, 2016

If you do change that though, make sure it would run in min mode, since now it runs in dev, but that should happen only at npm t time, otherwise linting the min source without running the tests in one, is... it also would be nice to keep running the suite without needing to build source, that is why i have "only at npm t time" note.

@dmethvin
Copy link
Member

dmethvin commented Mar 7, 2016

Changes LGTM here.

@mgol mgol closed this in f26c293 Mar 7, 2016
@mgol mgol deleted the drop-cruft branch March 7, 2016 23:20
@mgol
Copy link
Member Author

mgol commented Mar 7, 2016

Landed. 770 assertions before, 62 after. The new Migrate will be really new. ;)

@mgol
Copy link
Member Author

mgol commented Mar 9, 2016

I created #157 for the npm scripts refactor plan.

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.

5 participants