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

Regression when specifying a template_file_path #37

Closed
stefanceriu opened this issue Feb 23, 2024 · 5 comments
Closed

Regression when specifying a template_file_path #37

stefanceriu opened this issue Feb 23, 2024 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@stefanceriu
Copy link
Contributor

Context πŸ•΅οΈβ€β™€οΈ

Setup the Prefire example project to use a - template_file_path: PreviewTests.stencil, where PreviewTests.stencil is just a copy of the default template next to PreFireExample.xcodeproj

What 🌱

Specifying a template_file_path doesn't seem to be working anymore resulting in a does not exist or is not readable error

Proposal πŸŽ‰

Let's try to fix it maybe 😁

@stefanceriu
Copy link
Contributor Author

Okay, so the problem seems to be that the Prefire binary isn't aware of what the right path should be. I've managed to get it working here piggybacking on the config file URL but perhaps you have an idea for a cleaner solution.

@BarredEwe BarredEwe added bug Something isn't working good first issue Good for newcomers labels Feb 24, 2024
@BarredEwe
Copy link
Owner

Fixed in: #39

@stefanceriu
Copy link
Contributor Author

stefanceriu commented Mar 5, 2024

We need to reopen this because it still doesn't work for swfit packages e.g. element-hq/compound-ios#61 and neither does my original fix from here so I'm not entirely sure what the problem is.

We were relying on it to set default perceptual precission to 0.98 and I was able to work around it by specifying .snapshot(perceptualPrecision: 0.98) on the previews themselves.

@BarredEwe BarredEwe reopened this Mar 5, 2024
@stefanceriu
Copy link
Contributor Author

This might help, the configuration isn't picked up when running directly through xcodebuild either

xcodebuild -project 'PreFireExample.xcodeproj' -scheme 'PrefireExample' -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 15,OS=17.2' test

@stefanceriu
Copy link
Contributor Author

I got it working by using URL(filePath:) to build correct file paths but it's clear that the ConfigPathBuilder needs tweaking. I would suggest rewriting it to use URls instead of direct string manipulation.

     static func load(from configPath: String?, testTargetPath: String?, verbose: Bool) -> Config? {
-        let possibleConfigPaths = ConfigPathBuilder.possibleConfigPaths(for: configPath, testTargetPath: testTargetPath)
+        var possibleConfigPaths = [String]()
+
+        if let configPath {
+            possibleConfigPaths = [URL(filePath: configPath).appending(path: ".prefire.yml").path()]
+        }

         for path in possibleConfigPaths {
-            guard let configUrl = URL(string: Constants.fileMark + path),
-                  FileManager.default.fileExists(atPath: configUrl.path),
+
+            let configUrl = URL(filePath: path)
+
+            print("🟒 Checking url: \(configUrl)")
+
+            guard FileManager.default.fileExists(atPath: configUrl.path),
                   let configDataString = try? String(contentsOf: configUrl, encoding: .utf8) else { continue }

             if verbose {
🟒 Checking url: file:///Users/stefanceriu/Developer/Prefire/Example/.prefire.yml
🟒 The '.prefire' file is used on the path: /Users/stefanceriu/Developer/Prefire/Example/.prefire.yml

vs what the current version does

🟑 The '.prefire' file was not found by paths:
  - /Users/stefanceriu/Developer/Prefire/Example/PrefireExampleTests/.prefire.yml
  - Users/stefanceriu/Developer/Prefire/Example/.prefire.yml
  - /Users/stefanceriu/Developer/Prefire/Example/Users/stefanceriu/Developer/Prefire/Example/.prefire.yml

stefanceriu added a commit to stefanceriu/Prefire that referenced this issue Mar 9, 2024
- rewrite the ConfigPathBuilder to always build absolute (correct) paths to the various places where `.prefire.yml` can be
- update the tests to take this into account
- change the Config to always use filePaths
stefanceriu added a commit to stefanceriu/Prefire that referenced this issue Mar 11, 2024
- rewrite the ConfigPathBuilder to always build absolute (correct) paths to the various places where `.prefire.yml` can be
- update the tests to take this into account
- change the Config to always use filePaths
BarredEwe added a commit that referenced this issue Mar 11, 2024
Fixes #37 - Build correct config file paths, update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants