-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
Fix/test key separator #308
Fix/test key separator #308
Conversation
Could probably do something like |
I agree with @owenvoke that if we're making it specific, we might as well make it really specific. |
This raises another interesting point. Note that the runner doesn't include the @ symbol. This makes sense, because it is removed in the I'm not saying that we necessarily should, but its definitely a point worth raising. |
For the separator, how about using This then also solves the unlikely but possible situation of a folder with 4 '@' symbols. |
Let me know what you prefer, I will update my PR. |
Let's go with |
613e6e5
to
71742eb
Compare
|
||
uses(MyFailingCustomClassTest::class); | ||
|
||
test('custom traits can be used', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TestCase is no longer needed and will actually fail completely after the change to >>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just updated the tests to include >>>
in the folder name, as it seems to work on macOS. I think that keeping the test can help people having the same issue when they have some strange folder names as I had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, what would happen if you attempted to pull this repo on a Windows machine? Seen as > is a forbidden character.
71742eb
to
b17eb0d
Compare
Using |
@owenvoke I don't think it needs that, we just don't need the test class with >>> in the title. No developer can ever use > in a directory if they ever plan on supporting windows platforms, which is why I suggested it. That makes the test case unnecessary. Hope that makes some sort of sense. |
@lukeraymonddowning I guess that could be removed, same with the one ending in |
b17eb0d
to
c04d6d9
Compare
I removed the tests, this PR will keep the history of the bug in case anyone has some |
Lol, sorry @titouanmathis It would make sense to me to keep the first test, but have the |
Haha, my bad, I will re-add the first test 👍 |
@lukeraymonddowning Tests are not passing. But once tests are passing, feel free to merge and to release. A release requires changes on the changelog + 9133b88. |
I added the output of the 2 new tests to the |
This PR introduces a more specific separator for the tests keys to avoid matching a potential folder name. The separator is changed from
@
to@@@@
as it has more chance to not match anything in the test file absolute path. Maybe this value could be more complex?