jest-circus test failures #3770

aaronabramov · 2017-06-08T20:50:10Z

addresses all failing tests in integration_tests/__tests__/failures-test.js

handles native node assert module failures
expect.assertions(555)
minor formatting tweaks to make errors to be formatted the same way we do it with jasmine

aaronabramov · 2017-06-08T20:52:24Z

packages/jest-matchers/src/index.js

-expect.getState = () => global[GLOBAL_STATE].state;
+expect.getState = getState;
+expect.setState = setState;
+expect.extractExpectedAssertionsErrors = extractExpectedAssertionsErrors;


i brought in here. It used to be inside jasmine package, which i think is wrong place for it to be in, since it deals with many matcher implementation details (accessing state, formatting errors, etc.)

#3217 (comment)

@thymikee yeah! i agree for assert support, but expect.assertions is another story, since it's an expect feature and heavily coupled to matchers package

Oh, I looked at wrong code 😅
👍

aaronabramov · 2017-06-08T20:53:02Z

packages/jest-matchers/src/global_object.js

@@ -0,0 +1,48 @@
+/**


i extracted this from jest-matchers/src/index.js

Maybe we can call it jestMatchersObject? (btw would be nice to settle on naming convention for files)

@thymikee that's a good name! i'll use that.

and yeah, our file naming conventions are broken :(

Renaming them would be quite easy since we have Flow though

renaming is not hard. agreeing on the convention is :D

i only like undercores + everything lower case (dependency_resolver.js) because:

macOS is case insensitive, and case issues happen quite often ("works locally, breaks on CI", or "i renamed the file, but it's already added to git and git won't see my new name")

no one knows when to capitalize filenames (is it when exporting classes? or objects? or functions?) and it's always a mess

using camel case tho saves 1 character everywhere you'd type an underscore, and it looks more like what you'd name a variable in JS.

it's definitely unrelated to the PR though :)

Put the comment in relevant issue ;)

thymikee · 2017-06-08T20:54:50Z

packages/jest-circus/src/formatNodeAssertErrors.js

+ * @flow
+ */
+
+ import type {DiffOptions} from 'jest-diff/src/diffStrings';


Did you run prettier on this?

good call :)

thymikee · 2017-06-08T21:00:45Z

packages/jest-circus/src/legacy_code_todo_rewrite/jest-adapter-init.js

      break;
    }
  }
 };

+const _addExpectedAssertionErrors = (test: TestEntry) => {
+  const errors = extractExpectedAssertionsErrors();
+  errors.length && (test.status = 'fail');


I know it is shorter, but I think it would be more readble like this, no?

if (errors.length) { test.status = 'fail' }

i would disagree with readability, recently i find it very hard to navigate through 100 lines functions that do just 2 things, but i agree that this line messes up semantics pretty bad by using a logical operator as an if condition :)

Yeah, as long as we don't have pattern matching :D

thymikee · 2017-06-08T21:16:42Z

packages/jest-matchers/src/index.js

+  getMatchers,
+  getState,
+  setMatchers,
+  setState,


Since these are functions, not belonging to a class, wouldn't it be better to use this naming?

getMatchersState

setMatchersState

it's still namespaced within the matchers package though.
and with this naming the other two functions will become getMatchersMatchers then :D

thymikee

Looking good. Left some comments inlined.

cpojer · 2017-06-09T09:26:23Z

integration_tests/__tests__/failures-test.js

@@ -17,6 +17,25 @@ const normalizeDots = text => text.replace(/\.{1,}$/gm, '.');

 skipOnWindows.suite();

+const cleanupStackTrace = stderr => {


Can we make sure this function verifies that there is a stack trace for both and fails if one of them or both are missing? :)

we can't run both versions yet (jasmine and circus). And this is mostly for me. when i'm done with circus this will be removed, and the error is there only to make sure i don't forget to remove it after i'm done :)

cpojer · 2017-06-09T09:28:22Z

packages/jest-circus/src/utils.js

@@ -220,7 +220,7 @@ const _formatError = (error: ?Exception): string => {
  } else if (error.message) {
    return error.message;
  } else {
-    return String(error);
+    return `${String(error)} thrown`;


If you are turning it into a sentence, it needs to end with a ..

this is the way jasmine throws errors now, i just change it to this to be compatible with current snapshots

codecov-io · 2017-06-09T18:20:48Z

Codecov Report

Merging #3770 into master will decrease coverage by 0.43%.
The diff coverage is 22.78%.

@@            Coverage Diff             @@
##           master    #3770      +/-   ##
==========================================
- Coverage   58.09%   57.65%   -0.44%     
==========================================
  Files         191      194       +3     
  Lines        6708     6773      +65     
  Branches        6        6              
==========================================
+ Hits         3897     3905       +8     
- Misses       2808     2865      +57     
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-circus/src/formatNodeAssertErrors.js	`0% <0%> (ø)`
packages/jest-circus/src/utils.js	`0% <0%> (ø)`	⬆️
packages/jest-circus/src/state.js	`0% <0%> (ø)`	⬆️
.../src/legacy_code_todo_rewrite/jest-adapter-init.js	`0% <0%> (ø)`	⬆️
...st-matchers/src/extractExpectedAssertionsErrors.js	`13.33% <13.33%> (ø)`
packages/jest-matchers/src/jest-matchers-object.js	`80% <80%> (ø)`
packages/jest-matchers/src/index.js	`97.46% <88.88%> (+2.28%)`	⬆️

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 dec54a3...bab4eac. Read the comment docs.

* jest-cirtus failure tests * jest-circus failure messages

github-actions · 2021-05-13T13:13:50Z

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.

facebook-github-bot added the CLA Signed ✔️ label Jun 8, 2017

aaronabramov commented Jun 8, 2017

View reviewed changes

thymikee reviewed Jun 8, 2017

View reviewed changes

aaronabramov force-pushed the failure-messages-1 branch from 5866a13 to 005140f Compare June 8, 2017 20:55

thymikee reviewed Jun 8, 2017

View reviewed changes

thymikee approved these changes Jun 8, 2017

View reviewed changes

cpojer reviewed Jun 9, 2017

View reviewed changes

cpojer approved these changes Jun 9, 2017

View reviewed changes

jest-cirtus failure tests

f238a43

aaronabramov force-pushed the failure-messages-1 branch 2 times, most recently from 8a57c1c to 1dbfa55 Compare June 9, 2017 17:31

jest-circus failure messages

bab4eac

aaronabramov force-pushed the failure-messages-1 branch from 1dbfa55 to bab4eac Compare June 9, 2017 17:37

aaronabramov merged commit 328dcdd into jestjs:master Jun 9, 2017

aaronabramov deleted the failure-messages-1 branch June 9, 2017 18:39

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

jest-circus test failures (jestjs#3770)

48ec304

* jest-cirtus failure tests * jest-circus failure messages

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jest-circus test failures #3770

jest-circus test failures #3770

aaronabramov commented Jun 8, 2017

aaronabramov Jun 8, 2017

thymikee Jun 8, 2017

aaronabramov Jun 8, 2017

thymikee Jun 8, 2017

aaronabramov Jun 8, 2017

thymikee Jun 8, 2017

aaronabramov Jun 8, 2017

thymikee Jun 8, 2017

thymikee Jun 8, 2017

aaronabramov Jun 8, 2017

thymikee Jun 8, 2017

thymikee Jun 8, 2017

aaronabramov Jun 8, 2017

thymikee Jun 8, 2017

aaronabramov Jun 8, 2017

thymikee Jun 8, 2017

thymikee Jun 8, 2017

aaronabramov Jun 9, 2017

thymikee left a comment

cpojer Jun 9, 2017

aaronabramov Jun 9, 2017

cpojer Jun 9, 2017

aaronabramov Jun 9, 2017

codecov-io commented Jun 9, 2017

github-actions bot commented May 13, 2021

		@@ -17,6 +17,25 @@ const normalizeDots = text => text.replace(/\.{1,}$/gm, '.');

		skipOnWindows.suite();

		const cleanupStackTrace = stderr => {

jest-circus test failures #3770

jest-circus test failures #3770

Conversation

aaronabramov commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 9, 2017

Codecov Report

github-actions bot commented May 13, 2021