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

Explore deprecation of istanbul-api #321

Closed
coreyfarrell opened this issue Mar 6, 2019 · 10 comments · Fixed by #378
Closed

Explore deprecation of istanbul-api #321

coreyfarrell opened this issue Mar 6, 2019 · 10 comments · Fixed by #378
Assignees

Comments

@coreyfarrell
Copy link
Member

I'd like for us to consider deprecating istanbul-api in favor of consumers directly using the implementation modules (istanbul-lib-report, etc). The motivation is source-map@0.7.3 - the async constructor requires breaking API changes to istanbul-lib-source-maps and I'm not sure how to make istanbul-api work with it. One idea I have is to publish istanbul-api@3.0.0 and immediate deprecate that version only. This way we discourage new modules from using istanbul-api but we don't cause a deprecation warning for the existing major users. When we drop node.js 6 and upgrade source-map we would not update istanbul-api to be compatible.

According to npmjs.com istanbul-api was downloaded 5.1 million times last week. jest-cli was downloaded 4.5 million times. jest-cli is being split up so the istanbul-api dependency is moving to @jest/reporters. createReporter is the only thing jest uses from istanbul-api so I'm hoping we can convince them to use istanbul-lib-report directly.

Another large user is karma-coverage-istanbul-reporter at 700k downloads last week. fusion-cli had 15k last week, I'm not seeing any other dependents with a large number of downloads.

About the missing 200k istanbul-api downloads, I'm guessing jest-cli@14 and below is still getting significant downloads, that version uses the original istanbul module.

@bcoe
Copy link
Member

bcoe commented Mar 6, 2019

@SimenB could I get your thoughts on this topic?

@SimenB
Copy link
Member

SimenB commented Mar 6, 2019

Hey! 👋
Happy to switch to using whatever API you want Jest to use (preferably via a PR 😀)

@coreyfarrell
Copy link
Member Author

@SimenB I could give it a shot. I've run yarn && yarn test almost successfully. A couple tests failed with messages about hg which surprising since I do have hg installed (sorry I lost the scrollback log for the error). Doesn't matter though I've established baseline for test results on my system. The things I need to know:

  • When I edit packages/jest-reporters/src/coverage_reporter.ts, how do I rebuild jest-reporters only?
  • How can I run jest-reporters tests only?

Obviously when I think I have a functional change I'll expand my testing to everything but I need to be able to run more focused tests initially.

@SimenB
Copy link
Member

SimenB commented Mar 8, 2019

A couple tests failed with messages about hg which surprising since I do have hg installed (sorry I lost the scrollback log for the error).

The hg tests were fixed just a couple of days ago (jestjs/jest#8066), so in theory refreshing your fork should do it. If not, I'll take a look!

When I edit packages/jest-reporters/src/coverage_reporter.ts, how do I rebuild jest-reporters only?

You should run yarn watch, it will first build everything, then start a watcher that only rebuilds the files that changes. You might want to run yarn watch:ts in a separate terminal to get typechecks in watch mode as well (make sure to run it after the initial build done by yarn watch so it doesn't rebuild everything)

If you've already built everything, you can also do node scripts/watch.js to start the watcher.

How can I run jest-reporters tests only?

By doing yarn jest packages/jest-reporters (--watch) from the root of the repo. You'll probably want yarn jest coverage reporters (--watch) at some point to also run the integration tests 🙂

@coreyfarrell
Copy link
Member Author

@SimenB I just got around to looking at this again, I'm still getting the hg failures after updating:

Summary of all failing tests
 FAIL  e2e/__tests__/jestChangedFiles.test.ts (13.26s)
  ● gets changed files for hg

    Command failed: hg status -amnu --rev min((!public() & ::.)+.)^ /tmp/jest-changed-files-test-dir /tmp/jest-changed-files-test-dir/nested-dir /tmp/jest-changed-files-test-dir/nested-dir/second-nested-dir
    abort: empty revision range

      at makeError (node_modules/execa/index.js:174:9)

 FAIL  e2e/__tests__/onlyChanged.test.ts (83.086s)
  ● gets changed files for hg

    expect(received).toMatch(expected)

    Expected pattern: /PASS __tests__(\/|\\)file2.test.js/
    Received string:  "
    
      ● Test suite failed to run
    
        abort: empty revision range
    
    "

      299 | 
      300 |   ({stdout, stderr} = runJest(DIR, ['-o', '--changedFilesWithAncestor']));
    > 301 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
          |                  ^
      302 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
      303 | });
      304 | 

      at Object.toMatch (e2e/__tests__/onlyChanged.test.ts:301:18)
      at asyncGeneratorStep (e2e/__tests__/onlyChanged.test.ts:13:103)
      at _next (e2e/__tests__/onlyChanged.test.ts:15:194)
      at e2e/__tests__/onlyChanged.test.ts:15:364
      at Object.<anonymous> (e2e/__tests__/onlyChanged.test.ts:15:97)

My hg version is 4.4.2, not sure that matters.

@SimenB
Copy link
Member

SimenB commented Apr 9, 2019

I'm using 4.9, I'm not sure if it matters either... You've update to master, right? We had a bug with it fixed with jestjs/jest#8066

If yes, you can just ignore those failures, CI will catch them if an istanbul change, for whatever reason, breaks them 🙂

@coreyfarrell
Copy link
Member Author

@SimenB I think I have the correct code in place for jest-reporters, having an issue with typescript checking.

packages/jest-reporters/src/coverage_reporter.ts:102:43 - error TS2345: Argument of type 'string' is not assignable to parameter of type '"none" | "text-summary" | "clover" | "cobertura" | "html" | "json" | "json-summary" | "lcov" | "lcovonly" | "teamcity" | "text" | "text-lcov"'.

102         tree.visit(istanbulReports.create(_reporter, {}), reportContext);
                                              ~~~~~~~~~

packages/jest-cli/src/cli/index.ts:14:22 - error TS2307: Cannot find module '@jest/core'.

It seems that @types/istanbul-report declares the specific values which can be used. The broken branch is at https://github.com/coreyfarrell/jest/tree/reports-next. Note that I have no idea what to do about packages/jest-reporters/src/__tests__/coverage_reporter.test.js. For now I've just deleted that file.

@coreyfarrell
Copy link
Member Author

The above happened when I ran yarn from a clean working copy.

@SimenB
Copy link
Member

SimenB commented Apr 9, 2019

@coreyfarrell I'd love to chat about these things. Do we have some Istanbul chat? Like slack, discord etc? If not, wanna join Jest's? It's part of Reactiflux

@coreyfarrell
Copy link
Member Author

@SimenB we have slack, you can join at http://devtoolscommunity.herokuapp.com/ (the actual slack is https://node-tooling.slack.com/ channel #istanbuljs). If that's trouble I could join jest, just let me know where to go.

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 a pull request may close this issue.

4 participants