Add --skip-ui-tests parameter to tuist test command #2832
Conversation
Sources/TuistAutomation/ProjectMappers/PruneUITestsProjectMapper.swift
Outdated
Show resolved
Hide resolved
| @@ -22,6 +22,7 @@ final class AutomationGraphMapperProvider: GraphMapperProviding { | |||
| mappers.append( | |||
| TestsCacheGraphMapper(hashesCacheDirectory: testsCacheDirectory, config: config) | |||
| ) | |||
| mappers.append(CacheTreeShakingGraphMapper()) | |||
There was a problem hiding this comment.
This mapper is getting rid of the targets marked as prune.
There was a problem hiding this comment.
As I mentioned in one of the comments, I'd extract the logic of ensuring the project integrity into a new mapper and use that one instead.
Sources/TuistKit/GraphMappers/AutomationProjectMapperProvider.swift
Outdated
Show resolved
Hide resolved
Tests/TuistAutomationTests/ProjectMappers/PruneUITestsProjectMapperTests.swift
Outdated
Show resolved
Hide resolved
* main: Update the link to documented guidelines in pull request template (tuist#2833) `Dependencies.swift` documentation (tuist#2804) [Fix] Resources Targets should inherit their parents Deployment Version (tuist#2830) docs: add fila95 as a contributor (tuist#2831) 🗑 Remove 'CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER' key 📜 Update the beautiful changelog 🛠 Fix the failing DefaultSettingsProviderTests Deal with SPM 🙈 wip wip
| public final class PruneUITestsProjectMapper: ProjectMapping { | ||
| public init() {} | ||
|
|
||
| public func map(project: Project) throws -> (Project, [SideEffectDescriptor]) { |
There was a problem hiding this comment.
I think we should revisit the idea of "pruning". I think long-term, it's better if mappers just do what they are designed for. In this case removing certain targets for the project. This might leave the project in a invalid state, for example schemes with broken references to targets. For that reason I think we should extract from CacheTreeShakingGraphMapper the logic that ensures the integrity of the project. We can place that in a IntegrityGraphMapper. @luispadron do you think "integrity" is the right word for describe what it does?
There was a problem hiding this comment.
Yeah, it makes sense to move the tree-shaking to a mapper that will always be run as the last one to ensure the "integrity". Maybe ValidityChecker as an alternative to Integrity (just throwing ideas out there)?
There was a problem hiding this comment.
Hey @pepibumur , @fortmarek 👋
I like the idea, however maybe we can tackle that in the following PR from two reasons:
- I'd like to keep the size of a pull request small
- getting rid of puring feels out of scope for this PR
I have already been playing with extracting the integrity part from CacheTreeShakingGraphMapper. I'd like to write down a quick summary to be sure we are on the same page 🙏
SkipUITestsProjectMapperwould remove the ui tests targets explicitly instead of marking them as to be pruned.
public final class SkipUITestsProjectMapper: ProjectMapping {
public func map(project: Project) throws -> (Project, [SideEffectDescriptor]) {
var project = project
project.targets = project.targets.filter { $0.product != .uiTests }
return (project, [])
}
}-
IntegrityGraphMapperimplementation would be identical to the current state ofCacheTreeShakingGraphMapper, but it would do nothing related totarget.prune, meaning this would be removed:
-
We'd need to add
IntegrityGraphMapperas a last mapper to every place where theCacheTreeShakingGraphMapperis added. -
CacheGraphMutatorsets targets to be pruned - how we would migrate this part? Should the mutator remove the targets directly? Would we needCacheTreeShakingGraphMapperthen still? I am a bit confused how those would fit together.
Thanks for the feedback!
There was a problem hiding this comment.
Hope you do not mind, but I have created a follow-up ticket to tackle pruning refactoring 🙇
| @@ -46,6 +46,11 @@ struct TestCommand: ParsableCommand { | |||
| ) | |||
| var configuration: String? | |||
|
|
|||
| @Flag( | |||
There was a problem hiding this comment.
Nitpick: I'd extend the annotation to be more explicit about the short version of the flag. Since it's UI tests, what about making it -u?
There was a problem hiding this comment.
Hmm, if at some point we decided to add skip-unit-tests option, wouldn't a -u short version be confusing? 🤔 Maybe -ui?
|
I forgot to mention that we should update the documentation to document what the new flag is for. Moreover, I'd consider adding an acceptance test that verifies that UI tests are skipped. You can set up the acceptance test to run |
| public final class PruneUITestsProjectMapper: ProjectMapping { | ||
| public init() {} | ||
|
|
||
| public func map(project: Project) throws -> (Project, [SideEffectDescriptor]) { |
There was a problem hiding this comment.
Yeah, it makes sense to move the tree-shaking to a mapper that will always be run as the last one to ensure the "integrity". Maybe ValidityChecker as an alternative to Integrity (just throwing ideas out there)?
* main: (118 commits) Don't import the config Bump @theme-ui/color from 0.3.5 to 0.8.4 in /projects/website (tuist#2908) Fix documentation website Update docusaurus Bump @typescript-eslint/eslint-plugin in /projects/next (tuist#2929) Bump gatsby-image from 3.4.0 to 3.5.0 in /projects/next (tuist#2936) Bump babel-preset-gatsby from 1.4.0 to 1.5.0 in /projects/next (tuist#2939) Bump gatsby-image from 3.4.0 to 3.5.0 in /projects/website (tuist#2951) Bump gatsby-plugin-manifest from 3.4.0 to 3.5.0 in /projects/website (tuist#2952) Bump gatsby-plugin-mdx from 2.4.0 to 2.5.0 in /projects/next (tuist#2935) Bump prettier from 2.2.1 to 2.3.0 in /projects/website (tuist#2942) Bump eslint from 7.25.0 to 7.26.0 in /projects/next (tuist#2931) Bump @docusaurus/core in /projects/docs (tuist#2924) Bump @vitejs/plugin-react-refresh from 1.3.2 to 1.3.3 in /projects/lab (tuist#2923) Bump devise from 4.7.3 to 4.8.0 in /projects/lab (tuist#2926) Bump puma from 5.2.2 to 5.3.1 in /projects/lab (tuist#2928) Bump vite-plugin-full-reload from 0.2.1 to 0.2.2 in /projects/lab (tuist#2932) Bump rubocop from 1.13.0 to 1.14.0 in /projects/lab (tuist#2933) Bump rubocop-rails_config from 1.4.2 to 1.5.3 in /projects/lab (tuist#2934) Bump rails from 6.1.3.1 to 6.1.3.2 in /projects/lab (tuist#2937) ... # Conflicts: # CHANGELOG.md
|
This PR has been marked as stale because it's been opened for more than 30 days. If no action is taken, it'll be closed in 5 days. |
* main: (95 commits) fix typo on tuist graph command option --skip-test-targets Bump normalize-url from 4.5.0 to 4.5.1 in /projects/docs (tuist#3068) [Dependencies.swift] Generate `DependenciesGraph` for `Carthage` dependencies. (tuist#3043) Fix manifest loading when using Swift 5.5 (tuist#3062) Update project.md (tuist#3063) Add release name Bundle libProjectAutomation.dylib Support for tvOS Top Shelf Extensions (tuist#2793) Fix tuist and tuistenv release mismatch Version 1.44.0 Bump version to 1.44.0 Bump gatsby-plugin-theme-ui from 0.8.4 to 0.9.1 in /projects/website (tuist#3046) Bump autoprefixer from 10.2.5 to 10.2.6 in /projects/website Bump gatsby-plugin-sharp from 3.5.0 to 3.6.0 in /projects/website Bump react-spring from 9.1.2 to 9.2.1 in /projects/website Bump gatsby-redirect-from from 0.3.0 to 0.4.1 in /projects/website Bump @theme-ui/color from 0.8.4 to 0.9.1 in /projects/website Bump tailwindcss from 2.1.2 to 2.1.4 in /projects/website Bump ws from 6.2.1 to 6.2.2 in /projects/docs Remove stickers and shop link ... # Conflicts: # CHANGELOG.md
The documentation has been updated 👌
TBH I'd stick to unit tests only as most of the logic is contained. |
|
Hey @pepibumur , @fortmarek, @luispadron 👋 Thanks a lot for your feedback ❤️ I believe every comment / suggestion has been addressed and the pull request is ready for re-review 👀 |
projects/docs/docs/commands/test.md
Outdated
| @@ -57,3 +57,4 @@ One of the benefits of using Tuist over other automation tools is that developer | |||
| | `--device` | `-d` | `Test on a specific device.` | | No | | |||
| | `--os` | `-o` | `Test with a specific version of the OS.` | | No | | |||
| | `--configuration` | `-C` | `The configuration to be used when building the scheme.` | | No | | |||
| | `--skip-ui-tests` | `-C` | `When passed, it skips testing UI Tests targets.` | False | No | | |||
There was a problem hiding this comment.
I believe the short flag here is wrong.
There was a problem hiding this comment.
Fixed, by not allowing a short. The reason behind it mentioned here.
| @@ -265,15 +265,17 @@ private extension TestService { | |||
| configuration: String? = nil, | |||
| path: AbsolutePath, | |||
| deviceName: String? = nil, | |||
| osVersion: String? = nil | |||
| osVersion: String? = nil, | |||
| skipUiTests: Bool = false | |||
There was a problem hiding this comment.
Nit: There is inconsistent usage of skipUiTests and skipUITests - can you just do a quick search and replace, so only skipUITests is used?
Resolves #2562
Short description 📝
Add
--skip-ui-testsparameter totuist testcommand to skip UI Tests targets from being executed.The changes in a nutshell
new project mapper called
PruneUITestsProjectMapperhas been introduced. It marks UI Tests targets as "to be pruned" (only if a--skip-ui-testsflag is passed) and thenCacheTreeShakingGraphMapperremoves marked targets ✨Please let me know if you feel like more acceptance / unit tests should be added 🙏
Kudos to @fortmarek for help 😊
$ tuist test --skip-ui-testsChecklist ✅
CHANGELOG.mdhas been updated to reflect the changes. In case of a breaking change, it's been flagged as such.