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 build phases comparator #38

Conversation

marciniwanicki
Copy link
Contributor

@marciniwanicki marciniwanicki commented Dec 15, 2019

Partially resolves: #17

Describe your changes
Added a new comparator to compare lists of build phases from two projects. The comparison is slightly more complicated to give clear output for most common cases i.e. missing a single build phase in one of the projects, the same build phases but in different order, or different properties in one of the projects.

Testing performed

  • CI / unit and integration tests
  • Copy an existing project, modify list of build phases in the copy, run xcdiff -p1 <original> -p2 <copy> -g build_phases -v, make sure all the differences are listed.

@codecov
Copy link

codecov bot commented Dec 15, 2019

Codecov Report

Merging #38 into master will increase coverage by 0.19%.
The diff coverage is 97.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   94.82%   95.01%   +0.19%     
==========================================
  Files          39       40       +1     
  Lines        1663     1787     +124     
==========================================
+ Hits         1577     1698     +121     
- Misses         86       89       +3
Impacted Files Coverage Δ
Sources/XCDiffCore/Library/TargetsHelper.swift 93.37% <ø> (ø) ⬆️
...es/XCDiffCore/Library/Collections+Extensions.swift 83.33% <0%> (-16.67%) ⬇️
Sources/XCDiffCore/ComparatorType.swift 100% <100%> (ø) ⬆️
.../XCDiffCore/Comparator/BuildPhasesComparator.swift 100% <100%> (ø)

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 e000640...21bc496. Read the comment docs.

kwridan
kwridan previously approved these changes Jan 6, 2020
Copy link
Contributor

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

👍

As a suggestion, to help identify missing build phases we could still offer displaying "only in first" and "only in second"

via:

return result(context: context,
              first: Set(firstDescriptors.map { $0.description }),
              second: Set(secondDescriptors.map { $0.description }),
              differentValues: differentValues)

This would produce:

$ cd Fixtures
$ xcdiff -g build_phases -v

❌ BUILD_PHASES > "MismatchingLibrary" target

⚠️  Only in first (1):

  • name = CopyFiles, type = copyFiles, runOnlyForDeploymentPostprocessing = false


⚠️  Only in second (2):

  • name = Headers, type = headers, runOnlyForDeploymentPostprocessing = false
  • name = Resources, type = resources, runOnlyForDeploymentPostprocessing = false


⚠️  Value mismatch (4):

  • Build Phase 1
    ◦ name = Sources, type = sources, runOnlyForDeploymentPostprocessing = false
    ◦ name = Headers, type = headers, runOnlyForDeploymentPostprocessing = false

  • Build Phase 2
    ◦ name = Frameworks, type = frameworks, runOnlyForDeploymentPostprocessing = false
    ◦ name = Sources, type = sources, runOnlyForDeploymentPostprocessing = false

  • Build Phase 3
    ◦ name = CopyFiles, type = copyFiles, runOnlyForDeploymentPostprocessing = false
    ◦ name = Frameworks, type = frameworks, runOnlyForDeploymentPostprocessing = false

  • Build Phase 4
    ◦ nil
    ◦ name = Resources, type = resources, runOnlyForDeploymentPostprocessing = false


✅ BUILD_PHASES > "Project" target
✅ BUILD_PHASES > "ProjectFramework" target
✅ BUILD_PHASES > "ProjectTests" target
✅ BUILD_PHASES > "ProjectUITests" target

If we go for the onlyInFirst / onlyInSecond maybe we could summarise the value differences?

⚠️  Value mismatch (1):
   • Build Phase Order
    ◦ Sources, Frameworks, CopyFiles
    ◦ Headers, Sources, Frameworks, Resources

Guess there will always be some duplication, what you have is a good start either way to allow us to test this out further.

kwridan
kwridan previously approved these changes Jan 8, 2020
Copy link
Contributor

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

👍 Nice work

@@ -0,0 +1,176 @@
//
// Copyright 2019 Bloomberg Finance L.P.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: the copyright year is now 2020 :p

])
}

func testCompare_whenTwoBuildPhasesOfSameTypeOnlyInFirstSameOrder() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@marciniwanicki marciniwanicki force-pushed the feature/build-phases-comparator branch 2 times, most recently from cf81ebe to fad7650 Compare January 8, 2020 12:45
Copy link
Contributor

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

we can add specific copy phase / script phase comparators as follow ups

@marciniwanicki marciniwanicki merged commit 02c720d into bloomberg:master Jan 8, 2020
@marciniwanicki marciniwanicki deleted the feature/build-phases-comparator branch January 8, 2020 20:18
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.

Add app extensions comparator
2 participants