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

Shared system instance #519

Merged
merged 7 commits into from Oct 10, 2019
Merged

Shared system instance #519

merged 7 commits into from Oct 10, 2019

Conversation

pepicrft
Copy link
Contributor

@pepicrft pepicrft commented Sep 30, 2019

Short description 📝

Many methods in the code base took system: Systeming as an argument. That allowed us to inject a mock system for testing purposes but as a downside, it added unnecessary verbosity to the definition of the methods.

Solution 📦

Like we did with other utilities, this PR updates the class to make the constructor internal, and exposes a shared static instance. Since this class doesn't hold any state, in case we start doing more work in multiple threads in the future, it should be thread-safe.

Implementation 👩‍💻👨‍💻

  • Make the constructor internal.
  • Expose System.shared.
  • Add test helpers.
  • Fix tests.

@pepicrft pepicrft requested review from fortmarek and a team September 30, 2019 12:39
@pepicrft pepicrft self-assigned this Sep 30, 2019
Pedro Piñera added 2 commits October 10, 2019 08:55
…s to use those classes instead.

Make some progress

Refactor tests to use TuistUnitTestCase

Create TuistTestCase
@tuistbot
Copy link
Contributor

tuistbot commented Oct 10, 2019

1 Warning
⚠️ Have you introduced any user-facing changes? If so, please take some time to update the documentation. Keeping the documentation up to date makes it easier for users to learn how to use Tuist.

SwiftLint found issues

Warnings

File Line Reason
UpHomebrew.swift 49 Line should be 150 characters or less: currently 151 characters (line_length)

Generated by 🚫 Danger

@pepicrft pepicrft changed the title [WIP] Shared system instance Shared system instance Oct 10, 2019
@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #519 into master will increase coverage by 0.17%.
The diff coverage is 97.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   91.17%   91.34%   +0.17%     
==========================================
  Files         362      364       +2     
  Lines       19437    19478      +41     
==========================================
+ Hits        17722    17793      +71     
+ Misses       1715     1685      -30
Impacted Files Coverage Δ
Tests/TuistKitTests/Models/Up/UpTests.swift 100% <ø> (ø) ⬆️
...eTesting/Extensions/AargumentParser+TestData.swift 100% <ø> (ø)
Sources/TuistCore/Utils/System.swift 48.42% <ø> (-0.54%) ⬇️
...urces/TuistCoreTesting/Utils/MockFileHandler.swift 95.12% <ø> (+7.62%) ⬆️
...ing/Extensions/ArgumentParserResult+TestData.swift 100% <ø> (ø)
Tests/TuistGeneratorTests/Models/TargetTests.swift 100% <ø> (ø) ⬆️
Sources/TuistCoreTesting/Utils/MockSystem.swift 92.18% <ø> (+1.56%) ⬆️
...urces/TuistCoreTesting/Utils/MockEnvironment.swift 69.23% <ø> (+9.23%) ⬆️
...ts/Generator/StableStructureIntegrationTests.swift 99% <ø> (-0.02%) ⬇️
Sources/TuistCoreTesting/Utils/MockPrinter.swift 90% <ø> (+1.32%) ⬆️
... and 100 more

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 e695650...1163433. Read the comment docs.

@pepicrft pepicrft merged commit 47505e6 into master Oct 10, 2019
@pepicrft pepicrft deleted the shared-system branch October 10, 2019 09:28
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.

None yet

2 participants