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

Add testLocationInResults support to jest-circus #6291

Merged
merged 4 commits into from
May 27, 2018

Conversation

captbaritone
Copy link
Contributor

Part of #4362

@SimenB

@aaronabramov importing stack-utils in packages/jest-circus/src/utils.js breaks flow-strict. Thoughts?

// Technically the column should be 3, but callsites is not correct.
// jest-circus uses stack-utils + asyncErrors which resolves this.
expect(assertions[1].location).toEqual({
column: SkipOnJestCircus.isJestCircusRun() ? 3 : 2,
Copy link
Member

Choose a reason for hiding this comment

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

Nice solution!

unhandledErrors: unhandledErrors.map(_formatError),
};
};

const makeTestResults = (describeBlock: DescribeBlock): TestResults => {
const makeTestResults = (describeBlock: DescribeBlock, config): TestResults => {
const stackUtils = new StackUtils();
Copy link
Member

Choose a reason for hiding this comment

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

You can instantiate this at the top of the file. It will always be instantiated anyways

@@ -243,16 +248,25 @@ const makeTestResults = (describeBlock: DescribeBlock): TestResults => {
if (!status) {
throw new Error('Status should be present after tests are run.');
}

let location = null;
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 this needs to be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


let location = null;
if (config.testLocationInResults) {
const stackLine = test.asyncError.stack.split('\n')[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

@mjesun is gonna love it 😄 😄

@@ -102,7 +102,7 @@ export const runAndTransformResultsToJestFormat = async ({
globalConfig: GlobalConfig,
testPath: string,
}): Promise<TestResult> => {
const runResult = await run();
const runResult = await run(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm really not sure about passing the config to jest-circus world, that couples it pretty tightly to the rest of jest
would you be ok dispatching an action, like i did on line 73 in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@aaronabramov
Copy link
Contributor

@captbaritone seems like stack-utils doesn't have types, which makes its type any which is exactly what we're trying to catch :P
the solutions are:

  1. remove strict for now
  2. define types for stack utils and send a pr to flow-typed
  3. create a global .flow file in jest and put declare module 'stack-utils in there (and later send it upstream to flow-typed?)

@captbaritone
Copy link
Contributor Author

@aaronabramov I'm working on approach 3.

@@ -32,6 +32,9 @@ export type Hook = {
export type EventHandler = (event: Event, state: State) => void;

export type Event =
| {|
Copy link
Contributor

Choose a reason for hiding this comment

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

later if we want to keep jest-circus decoupled we should probably introduce config or settings property on the state and mutate them through a single action.
At the same time i'm a little worried about introducing one more config :D

@captbaritone
Copy link
Contributor Author

Flow types: #6294

@captbaritone
Copy link
Contributor Author

@aaronabramov rebased on top of #6294.

@codecov-io
Copy link

codecov-io commented May 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@5acaf11). Click here to learn what that means.
The diff coverage is 15.38%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6291   +/-   ##
=========================================
  Coverage          ?   63.47%           
=========================================
  Files             ?      221           
  Lines             ?     8513           
  Branches          ?        4           
=========================================
  Hits              ?     5404           
  Misses            ?     3108           
  Partials          ?        1
Impacted Files Coverage Δ
packages/jest-circus/src/state.js 92.3% <ø> (ø)
packages/jest-circus/src/event_handler.js 16.07% <0%> (ø)
.../src/legacy_code_todo_rewrite/jest_adapter_init.js 0% <0%> (ø)
packages/jest-circus/src/utils.js 21.62% <22.22%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5acaf11...064827a. Read the comment docs.

@aaronabramov aaronabramov merged commit 75831af into jestjs:master May 27, 2018
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants