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

Unique identifiers for all test examples #771

Closed

Conversation

jwfriese
Copy link
Contributor

@jwfriese jwfriese commented Feb 7, 2018

This PR addresses #770 by doing two things:

  1. Assigning unique identifiers to all examples based on the description chain of the example, adding numbers at the end to deduplicate if necessary.
  2. Elevating pending elements to full-fledged example objects that print these unique identifiers but still do not run their content.

There are a couple specific things that I would appreciate feedback on:

  • For the sake of trying to change one thing at a time (at least in spirit), I changed the Filter.pending flag to Filter.excluded. This is because I did not know whether there was some strong reason that xit and the like were removed entirely from the Quick test list as opposed to just neutering their content. I'll lean on @modocache and @jeffh to hopefully provide any history they can. It makes sense to me and would simplify the code to just treat x-type examples the same as pending, but I don't want to make that decision alone.

    Also, @shx7, will you need those xit, xdescribe, etc examples to print Pending: blah_blah_blah as well? If so, then we'll definitely have to make some more changes. Fortunately, I already experimented with that and it will be easy for me to do. I just need to know whether that's what we want.

  • Tests only start appending digits once its apparent they are duplicated. In other words, a set of tests which happen to be duplicated will have the following identifiers:

test_with_duplicated_identifier
test_with_duplicated_identifier_2
test_with_duplicated_identifier_3

Notice that the first one does not have a 1 appended to it. @shx7 will that satisfy the ask?

I still have a couple things to do on this PR, but wanted to start a review of the code and a conversation about its progress ASAP. I'll maintain the list here:

  • Prepend unique identifiers with test class name
  • (maybe) Refactor xit and its like elements to use the new pending code

Cheers, y'all.

@QuickBot
Copy link

QuickBot commented Feb 7, 2018

1 Warning
⚠️ Big PR

Generated by 🚫 danger

@jwfriese
Copy link
Contributor Author

I've started updating this PR as well, similar to the other two (#767 and #766).

@jwfriese jwfriese force-pushed the 770-more-specific-pending-test-output branch from d27a99a to 6f95c8c Compare January 1, 2019 18:58
) {}
groupContainingExamplesDuplicatedAcrossGroupsA.appendExample(exampleDupedAcrossSpecA)

let groupContainingExamplesDuplicatedAcrossGroupsB = ExampleGroup(

Choose a reason for hiding this comment

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

Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'groupContainingExamplesDuplicatedAcrossGroupsB' (identifier_name)


rootExampleGroup.appendExampleGroup(groupContainingExamplesWithDuplicateDescriptions)

let groupContainingExamplesDuplicatedAcrossGroupsA = ExampleGroup(

Choose a reason for hiding this comment

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

Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'groupContainingExamplesDuplicatedAcrossGroupsA' (identifier_name)

) {}
rootExampleGroup.appendExample(rootExample)

let groupContainingExamplesWithDuplicateDescriptions = ExampleGroup(

Choose a reason for hiding this comment

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

Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'groupContainingExamplesWithDuplicateDescriptions' (identifier_name)

import Nimble

class ExampleTests: XCTestCase {
func test_uniqueIdentifier_uniquelyIdentifiesTheExampleAcrossASpec() {

Choose a reason for hiding this comment

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

Function Body Length Violation: Function body should span 40 lines or less excluding comments and whitespace: currently spans 84 lines (function_body_length)

raiseError("'pending' cannot be used inside 'afterEach', 'pending' may only be used inside 'context' or 'describe'. ")
}
guard currentExampleMetadata == nil else {
raiseError("'pending' cannot be used inside 'it', 'pending' may only be used inside 'context' or 'describe'. ")

Choose a reason for hiding this comment

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

Line Length Violation: Line should be 120 characters or less: currently 123 characters (line_length)

raiseError("'pending' cannot be used inside 'beforeEach', 'pending' may only be used inside 'context' or 'describe'. ")
}
if aftersCurrentlyExecuting {
raiseError("'pending' cannot be used inside 'afterEach', 'pending' may only be used inside 'context' or 'describe'. ")

Choose a reason for hiding this comment

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

Line Length Violation: Line should be 120 characters or less: currently 130 characters (line_length)

@nonobjc
internal func pending(_ description: String, file: String, line: UInt, closure: @escaping () -> Void) {
if beforesCurrentlyExecuting {
raiseError("'pending' cannot be used inside 'beforeEach', 'pending' may only be used inside 'context' or 'describe'. ")

Choose a reason for hiding this comment

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

Line Length Violation: Line should be 120 characters or less: currently 131 characters (line_length)

@jwfriese
Copy link
Contributor Author

jwfriese commented Jan 1, 2019

@ikesyo - The file with the "c99 identifier" behavior is the only one that had to significantly change as part of my work to rebase this back into a happy state with master. I think that my accommodations as part of the merge keep in good faith with the changes that originally were made in there, but please feel free to suggest or discuss any concerns you have or things you'd like to see done differently in there.

@jwfriese
Copy link
Contributor Author

jwfriese commented Jan 3, 2019

For what it's worth, I don't think that CI failure is legit. It passed for all but one macOS config, and both Linux configs. Something went wrong with Ruby land in there.

@ikesyo
Copy link
Member

ikesyo commented Feb 3, 2019

This is related to #490 and #491.

@ikesyo ikesyo linked an issue Oct 8, 2020 that may be closed by this pull request
Base automatically changed from master to main March 2, 2021 13:54
@jessesquires
Copy link
Member

Hey there! 👋🏼

I'm a new maintainer for this project and I'm trying to get the next release out ASAP and also clean up old issues and old PRs. I'm closing all PRs older than 1-2 years, but that doesn't mean these changes are necessarily being rejected. If you are still interested in contributing these changes, please submit a new PR branching from the top of main.

We appreciate your work here and acknowledge the time and effort you put in to contribute! You're awesome. 💯 However, we are all volunteers here with limited capacity working for free. Unfortunately, that means we must close out stale issues and PRs to move the project forward in a reasonable way.

We apologize for the inconvenience and appreciate your understanding! 😎 ✌🏼

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 this pull request may close these issues.

Using pending causes custom Configuration hooks not to run
5 participants