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

indent test contexts #2814

Merged
merged 2 commits into from
Sep 29, 2017
Merged

indent test contexts #2814

merged 2 commits into from
Sep 29, 2017

Conversation

charlierudolph
Copy link
Contributor

Joining the all the test contexts by spaces makes it hard to parse. I've never attempted to read that string as its typically impossible to tell where one title ends and another begins.

screen shot 2017-05-20 at 12 08 38 pm

becomes

screen shot 2017-05-20 at 12 09 10 pm

If this will be accepted, I'm happy to work to remove fullTitle() from its other uses but for now just concentrated on this one that effects me.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 88.35% when pulling aa24e82 on charlierudolph:cr-indentContexts into e249434 on mochajs:master.

@ScottFreeCode
Copy link
Contributor

While considering any changes to title reporting we will need to keep in mind compatibility with tools such as karma-mocha and potential future support of https://github.com/js-reporters/js-reporters in addition to any general expectations of parity with other test systems or backwards-compatibility (whether with custom reporters or with anything that might be processing reporter output).

@charlierudolph
Copy link
Contributor Author

Looking at js-reporters it has the following for a given test:

fullName: Array - array of strings containing the name of the test and the names of all its suites ancestors.

We could deprecate fullTitle and introduce titlePath as is or with a new name.

@boneskull
Copy link
Contributor

@charlierudolph Thanks.

Yes, let's leave fullTitle() as-is, as @ScottFreeCode suggests. I think this change is probably a good one, but it's semver-major.

I'd encourage you to use this in a custom reporter for now, as I'm not sure when the next major will land.

@boneskull boneskull added pr-needs-work semver-major implementation requires increase of "major" version number; "breaking changes" labels Jun 3, 2017
@charlierudolph
Copy link
Contributor Author

@boneskull sounds good.

I just resolved conflicts but am otherwise curious as to what caused the addition of the pr-needs-work label

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 88.456% when pulling 296cb23 on charlierudolph:cr-indentContexts into f20de56 on mochajs:master.

@boneskull
Copy link
Contributor

@charlierudolph it looked to me like there were changes that @ScottFreeCode wanted, but maybe I was incorrect on that. I'll let him comment and/or remove the tag

@ScottFreeCode
Copy link
Contributor

I'm actually not sure I know the stuff I brought up well enough to make any definitive statements about impact to this PR; I just wanted to make sure present compatibility (and in some cases desired future compatibility) with those various things is taken into consideration.

@charlierudolph
Copy link
Contributor Author

@ScottFreeCode so is there someone else we should have look at this? Is there any path forward for this PR or are we blocked until the present compatibility is determined so we know what actions need to be taken with regard to this PR?

@ScottFreeCode
Copy link
Contributor

I believe the @mochajs/core team is still discussing where to merge semver-major changes into. I'm expecting a new semver-major branch for this purpose, but I don't know if we reached consensus on that.

If there are no other concerns from the team then that should be all we need. (Besides which, we could always revisit the details in between merging this in as-is and releasing the next semver-major.)

@charlierudolph
Copy link
Contributor Author

So does this PR still need the label pr-needs-work? Based on the current comments I think it needs a different label

@ScottFreeCode
Copy link
Contributor

Sorry, I missed "and/or remove the tag" in boneskull's comment when I read it before; I've gone ahead and removed it.

sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
Add an optional extended description…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants