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 a description with the reason why a test is pending #3324

Closed
wants to merge 12 commits into from

Conversation

rdennis
Copy link

@rdennis rdennis commented Apr 13, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

Added a reason property to Runnable and an optional reason parameter for it, specify, xit, xspecify, and it.skip. Also added the reason to reporters that made sense.

Alternate Designs

I considered a .because or .reason method that would accept the reason a test is marked pending, but it would require a larger change and can still be done after these changes.

Why should this be in core?

It is adding functionality to the core system.

Benefits

It will allow users to give a reason tests are pending. This functionality is helpful in that it lets teams track the status of pending tests in the test report. Right now, if a test is pending, it has to be documented in the test itself, or by some other external means.

Possible Drawbacks

It is possible that this implementation breaks existing tests in some case I have not considered. If those are found, it may be better to use a .reason or .because method instead.

Applicable issues

I have seen this functionality requested in #2026 and #2572.
This is an enhancement (minor release).

@jsf-clabot
Copy link

jsf-clabot commented Apr 13, 2018

CLA assistant check
All committers have signed the CLA.

@boneskull boneskull requested review from Bamieh and outsideris and removed request for Bamieh April 18, 2018 18:17
Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

While I think this feature is useful, I'm feeling modification of it's signature--and also the Runnable and Test constructors--is not on the table.

Instead, perhaps this.skip() could accept a reason argument. Then, we wouldn't need to modify each interface nor the constructors. Please see my code comments for further discussion.

Because I generally see it.skip() used in an ad-hoc, temporary manner, let's avoid modifying that API for now, and see if there's a pressing need, or if a change to this.skip() will be sufficient.

@@ -155,7 +155,8 @@ XUnit.prototype.test = function (test) {
var err = test.err;
this.write(tag('testcase', attrs, false, tag('failure', {}, false, escape(err.message) + '\n' + escape(err.stack))));
} else if (test.isPending()) {
this.write(tag('testcase', attrs, false, tag('skipped', {}, true)));
var reasonTag = test.reason ? tag('reason', {}, false, '<![CDATA[' + test.reason + ']]>') : '';
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 going to assume "reason" is a real thing per the XUnit spec? (I don't use XUnit, so I don't know)

Copy link
Contributor

Choose a reason for hiding this comment

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

@boneskull yes, in XUnit.net [Fact(Skip="reason")] or [Ignore("reason")] are used to skip a Fact/Test and provide a reason for skipping.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Bamieh happy to have someone around that actually uses XUnit 😄

@@ -83,7 +83,8 @@ function clean (test) {
fullTitle: test.fullTitle(),
duration: test.duration,
currentRetry: test.currentRetry(),
err: cleanCycles(err)
err: cleanCycles(err),
reason: test.reason
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Can we suppress addition of the reason property in the case of a non-pending test?
  2. Can we coerce reason to a String (e.g. reason: String(test.reason))

@@ -78,12 +78,16 @@ module.exports = function (suite) {
* acting as a thunk.
*/

context.it = context.specify = function (title, fn) {
context.it = context.specify = function (title, fn, reason) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see my review summary regarding this API.

lib/runnable.js Outdated
*/
function Runnable (title, fn) {
function Runnable (title, fn, reason) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert changes to this file (see review summary)

lib/test.js Outdated
@@ -20,12 +20,13 @@ module.exports = Test;
* @api private
* @param {String} title
* @param {Function} fn
* @param {String} reason
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert changes to this constructor. Even if reason must be a property of a Test or Runnable (I'm not entirely convinced), it should be renamed pendingReason and settable/gettable via methods.

Please investigate using the reason as the message of a thrown Pending object instead. I see at least one place where the Runner emits pending where we have access to the Pending instance itself... that might be all we need. Then, we can do something like this.emit('pending', test, pending.message) and the reporter can handle it.

@@ -32,6 +46,25 @@ describe('pending', function () {
done();
});
});
it('should accept a reason when used via shorthand methods', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

tests look great 👍

@boneskull
Copy link
Contributor

(Also this needs to be rebased onto master w/ conflicts resolved)

@outsideris outsideris added type: feature enhancement proposal area: usability concerning user experience or interface labels Apr 19, 2018
@rdennis
Copy link
Author

rdennis commented Apr 23, 2018

Thanks for the feedback, I'll make these changes.

@boneskull boneskull force-pushed the master branch 2 times, most recently from 4547268 to 7613521 Compare May 4, 2018 16:47
@pdulapalli
Copy link

Hello, I was curious about the status of this PR. This is something I would really like implemented in Mocha.

Would it be possible for me to make some of the changes you've recommended @rdennis to make?

@rdennis rdennis reopened this May 19, 2018
@rdennis
Copy link
Author

rdennis commented May 19, 2018

I think I've made all of the requested changes. The best source I could find for the xunit XML format (https://xunit.github.io/docs/format-xml-v2) implies a completely different format from that of the current xunit reporter, so I didn't bother adding reason to it this time. If anyone can find me an authoritative source for what the current reporter should output to properly display reason, I'll gladly add it.

lib/runnable.js Outdated
if (arguments.length > 0) {
reason = String(reason);
this.reason = reason;
}
Copy link
Contributor

@outsideris outsideris May 19, 2018

Choose a reason for hiding this comment

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

Why just check if (reason) {?

In a case ofit.skip(), arguments is { '0': undefined } and arguments.length is 1, because of this.runnable().skip(reason) in lib/context.js.
Therefore, reason is 'undefined' as string and it print like this:

- should skip immediately with reason (undefined)

Copy link
Author

Choose a reason for hiding this comment

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

Ah, good catch. Originally I has if (reason) {, but thought this would be better for some reason. I'll change it back.

Copy link
Author

@rdennis rdennis May 19, 2018

Choose a reason for hiding this comment

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

I also considered using utils.isString to validate the input and throw an exception if invalid, but wasn't sure if that was needed here.

Copy link
Contributor

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

undefinded is printed as string.

@outsideris
Copy link
Contributor

I didn't use xunit as well.
Jenkins's junit xsd looks same with our format.

I guess we can use message in skipped element for skip reason.

<xs:element name="skipped">
    <xs:complexType mixed="true">
        <xs:attribute name="type" type="xs:string" use="optional"/>
        <xs:attribute name="message" type="xs:string" use="optional"/>
    </xs:complexType>
</xs:element>

@outsideris
Copy link
Contributor

Additionally, it will be awesome if you add about reason in document.
Maybe here?

@rdennis
Copy link
Author

rdennis commented May 20, 2018

Ok, I added support for before and beforeEach, but it required not passing a default message to new Pending in Runnable::skip (sync and async). I'm not sure if this is an acceptable change as I'm not sure what side effects it may have. Also, it just feels a little weird having before give the reason to every test in the suite. Maybe it just shouldn't be supported, but it seems counter intuitive to allow this.skip(reason) in spec, but not in before or beforeEach.

@plroebuck
Copy link
Contributor

I hate to ask this (seeing how much work you invested), but why couldn't this problem be handled like this?

describe('suite description', function() {
  context('reason why this test is pending', function() {
    it.skip('test description');
  });
});

Though not as customized as your suggestion, would seem to give much of the same effect without any code changes to Mocha.

@craigtaub
Copy link
Contributor

@rdennis have u had any time to look at this again?

@boneskull boneskull added the stale this has been inactive for a while... label Dec 18, 2018
@stale stale bot closed this May 4, 2019
@JoshuaKGoldberg
Copy link
Member

👋 coming back to this @rdennis, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface stale this has been inactive for a while... type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants