Skip to content

Refactoring after fixing test cases discoverage in Xcode 13.3#1134

Open
BobCatC wants to merge 6 commits intoQuick:mainfrom
BobCatC:xcode_13.3_testBundleWillStart_step2
Open

Refactoring after fixing test cases discoverage in Xcode 13.3#1134
BobCatC wants to merge 6 commits intoQuick:mainfrom
BobCatC:xcode_13.3_testBundleWillStart_step2

Conversation

@BobCatC
Copy link
Copy Markdown
Contributor

@BobCatC BobCatC commented Apr 14, 2022

Intro

Next step after #1129

As we agreed with @jessesquires this is a refactoring PR after small fix of Xcode 13.3 issue. What code was refactored read below.

AllExamplesBuilder class

Just extracted logic of calling method buildExamplesIfNeeded on each QuickSpec subclass from QuickTestObservation into separate class.
You can note that the instance has a flag - didBuildAllExamples which helps to execute method only once per lifetime of the object. And also you can note that the object is used as a property of World instance. So its lifetime equals world's lifetime (which in real projects acts like a singleton).

Tests for the issue

I've added a test for Xcode 13.3 issue. And to be able to test that behaviour I had to add into function qck_runSpec new param - gatherExamples. Passing false simulates Xcode 13.3 problem, passing true (which is default) behaves like before.

Checklist

  • Does this have tests? Yes
  • Does this have documentation? No (needed?)
  • Does this break the public API (Requires major version bump)? No
  • Is this a new feature (Requires minor version bump)? No

BobCatC added 6 commits April 14, 2022 23:21
…of AllExamplesBuilder (and flag 'didBuildAllExamples') depends on the lifetime of world
…d class) into #if !SWIFTP_PACKAGE directive because this way of initialization is used only in ObjC version of QuickSpec which is not for SPM
@BobCatC
Copy link
Copy Markdown
Contributor Author

BobCatC commented Apr 14, 2022

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@BobCatC
Copy link
Copy Markdown
Contributor Author

BobCatC commented Apr 14, 2022

Danger fails as usual because I'm not a contributor. Ran it locally - it's ok (see comment above).

Copy link
Copy Markdown
Member

@jessesquires jessesquires left a comment

Choose a reason for hiding this comment

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

Thanks @BobCatC ! 💯

I think this looks good.

One request:

Can you comment to call out what was simply moved vs what was changed? I think I understand what's happening, but this would be very helpful — and good for posterity.

@jessesquires jessesquires added this to the v5.0.0 milestone Apr 14, 2022
@jessesquires
Copy link
Copy Markdown
Member

@BobCatC oh also, I've marked this for the v5.0 release.

This would be the final PR to merge for that.

It doesn't seem very risky, but I would appreciate your thoughts on the risk level as well.

@jessesquires
Copy link
Copy Markdown
Member

@BobCatC one last question. Does this also address #1115?

}

// swiftlint:disable:next todo
// TODO: Unify this with QuickConfiguration's equivalent
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should address this before merging.

But I don't have a ton of context here.

@jessesquires
Copy link
Copy Markdown
Member

Hey @younata -- not sure if you saw, but GitHub restored all the previously-deleted Russian account contributions. So this PR is back now. 😊 https://www.jessesquires.com/blog/2022/04/19/github-suspending-russian-accounts/#updated-21-april-2022

@younata younata modified the milestones: v6.0.0, v7.0.0 Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants