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

Some Windows fixes #41

Closed
wants to merge 5 commits into from
Closed

Some Windows fixes #41

wants to merge 5 commits into from

Conversation

Pierstoval
Copy link
Contributor

@Pierstoval Pierstoval commented May 28, 2020

Binary was using realpath, and targets were sanitized without taking the full path's drive name in account, so this was fixed.

I intend to wait for CI to make sure the test suite still works as expected.

I hope it'll fix #36 🤔

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

This sounds good to me. A second validation would be nice here, @olivernybroe @owenvoke ?

Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

This looks ok to me. Damn Windows support. sigh I've tested it locally on my Windows machine and most of the tests are successful. 👍

However, as per #37, the tests still aren't actually being executed here on Windows despite the CI passing. 👍


I'll have another look in the morning though.

Ok, I've re-tested on my proper Windows dev machine. The following tests seem to fail:

The first 3 tests fail due to not having the trait methods available, so I don't think uses() is working as expected. 🤔

The visual tests fail with the following:

Failed asserting that '' contains " PASS Tests\Fixtures\DirectoryWithTests\ExampleTest


@Pierstoval, were all tests passing on your Windows machine? 🤔

@nunomaduro
Copy link
Member

Screenshot 2020-05-29 at 20 11 56

@nunomaduro
Copy link
Member

@Pierstoval Can you rebase buddy? The fix of the build is now on the master.

@nunomaduro
Copy link
Member

Yup, couple of tests on windows are failing. This is good sign.

@Pierstoval
Copy link
Contributor Author

Locally, Pest tests work, but i have some that fail:

   WARN  EdevwwwpesttestsFeaturesSkip
  ✓ it do not skips
  s it skips with truthy
  s it skips with truthy condition by default
  s it skips with message → skipped because bar
  s it skips with truthy closure condition
  ✓ it do not skips with falsy closure condition
  s it skips with condition and message → skipped because foo

   FAIL  EdevwwwpesttestsPHPUnitCustomTestCaseInSubFoldersSubFolderSubFolderUsesPerSubDirectory
  • closure was bound to  custom test case

   FAIL  EdevwwwpesttestsPHPUnitCustomTestCaseUsesPerDirectory
  • closure was bound to  custom test case

   FAIL  EdevwwwpesttestsPluginsTraits
  • it allows global uses

   FAIL  EdevwwwpesttestsVisualSingleTestOrDirectory
  • allows to run a single test
  • allows to run a directory

(sorry, output is not really easy to copy to github 😅 )

@owenvoke
Copy link
Member

Yeah, I was trying to work those out earlier. 🤔 (#41 (review))

@Pierstoval
Copy link
Contributor Author

Pierstoval commented May 29, 2020

Some tests are related to the fact that php isn't called, and bin/pest is, I'm investigating :)

@Pierstoval
Copy link
Contributor Author

Another issue is how test names are displayed in the output, I've got this issue after a fix:

 $ php bin/pest tests\Visual\SingleTestOrDirectory.php

   FAIL  EdevwwwpesttestsVisualSingleTestOrDirectory
  • allows to run a single test
  • allows to run a directory

  ---

  • EdevwwwpesttestsVisualSingleTestOrDirectory > allows to run a single test
  Failed asserting that '\n
     PASS  TestsFixturesDirectoryWithTestsExampleTest\r\n
    ✓ it example\r\n
  \n
    Tests:  1 passed\r\n
    Time:   0.04s\r\n
  ' contains "   PASS  Tests\Fixtures\DirectoryWithTests\ExampleTest
    ✓ it example

    Tests:  1 passed".

As you can see, the test name should be Tests\Fixtures\DirectoryWithTests\ExampleTest, but it's displayed as TestsFixturesDirectoryWithTestsExampleTest.

I'm trying to search where this could be done... @nunomaduro, any idea?

@Pierstoval
Copy link
Contributor Author

Pierstoval commented May 29, 2020

I made a few changes, it's still investigation more than bug fixes since I'm only testing this on Windows.

(EDIT: At least, tests are executing in most Windows CI!)

I'm totally unsure of 6fb3c79 since I don't know what's going to be the impact on Linux 🤔 but I was unsure about using basename() since it didn't display the full test name, instead was just displaying the class name and not its FQN in the test output.

By default, with basename(), I get this:

$ php bin/pest tests\Visual\SingleTestOrDirectory.php

   FAIL  SingleTestOrDirectory
  • allows to run a single test
  • allows to run a directory

  ---

  • SingleTestOrDirectory > allows to run a single test
  Failed asserting that '\n
     PASS  ExampleTest\r\n
    ✓ it example\r\n
  \n
    Tests:  1 passed\r\n
    Time:   0.04s\r\n
  ' contains "   PASS  Tests\Fixtures\DirectoryWithTests\ExampleTest
    ✓ it example

    Tests:  1 passed".

If I replace basename() with the preg_replace() in 6fb3c79 , I get this:

$ php bin/pest tests\Visual\SingleTestOrDirectory.php

   FAIL  Tests\Visual\SingleTestOrDirectory
  • allows to run a single test
  • allows to run a directory

  ---

  • Tests\Visual\SingleTestOrDirectory > allows to run a single test
  Failed asserting that '\n
     PASS  E\dev\www\pest\tests\Fixtures\DirectoryWithTests\ExampleTest\r\n
    ✓ it example\r\n
  \n
    Tests:  1 passed\r\n
    Time:   0.04s\r\n
  ' contains "   PASS  Tests\Fixtures\DirectoryWithTests\ExampleTest
    ✓ it example

    Tests:  1 passed".

There seem to be tons of things related to paths and root dir, I'm not sure where to search next🤔

@Pierstoval Pierstoval marked this pull request as draft May 29, 2020 22:16
@Pierstoval
Copy link
Contributor Author

(marked it as draft because it seems it's definitely NOT ready to be merged)

@@ -152,15 +152,23 @@ public function build(TestSuite $testSuite): array
*/
public function makeClassFromFilename(string $filename): string
{
if ('\\' === DIRECTORY_SEPARATOR) {
Copy link
Member

Choose a reason for hiding this comment

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

This will only happen on windows?

Copy link
Member

Choose a reason for hiding this comment

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

Might be better to just use PHP_OS_FAMILY === 'Windows'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this can happen only on Windows, that's a fairly common case 🙂

@nunomaduro
Copy link
Member

@Pierstoval Hey buddy, you are still working on this correct? Its fine if takes weeks, I just wanna make sure that there is no action on my side to be done.

@Pierstoval
Copy link
Contributor Author

For now, I need to check why tests are failing, but I'm a bit busy these days :)

@nunomaduro
Copy link
Member

No problem. Going to mark this issue as needs help. Maybe someone can take over.

@dimitrioskarvounaris
Copy link
Contributor

So, what exactly is here more required? Can you elaborate a little more?

@nunomaduro
Copy link
Member

Sure, normally if you are on windows you can reproduce the problem running: composer test:unit.

@Pierstoval
Copy link
Contributor Author

@dimitrioskarvounaris The tests must be fully working on Windows, which is not the case now

@dimitrioskarvounaris
Copy link
Contributor

Continuing work under https://github.com/dimitrioskarvounaris/pest/tree/windows-gitbash
I got the pull request with the github-cli and then tried to push it to my repo to not create separate branches and maybe add it on top of this one, but it will keep asking permissions to Pierstoval's repo to commit on the same branch, even though remote url points to my fork. If you know a workaround to this, let me know.

@Pierstoval
Copy link
Contributor Author

@dimitrioskarvounaris feel free to make PRs on my fork to add your commits to this PR, I will accept them right away without review so they can be reviewed right here instead :)

@dimitrioskarvounaris
Copy link
Contributor

dimitrioskarvounaris commented Jun 2, 2020

Felt it was too much trouble, to have one more fork (if that would even work at all, to make a fork from a fork, while having another fork of the original repo), and github wouldn't let me push to the same branch even on my fork, as I used the github-cli to clone the pull request. Github docs also said, I would need still push permissions on the other fork... sorry... so I just made a new PR here instead of going through all the trouble: #61 - it contains though all previous commits too. This way, I can use the extra time I would have lost trying that (and maybe still not working) in fixing the integration tests, instead.

@nunomaduro
Copy link
Member

This is the branch that contains the windows work? If yes, can you close the other one @dimitrioskarvounaris?

@nunomaduro
Copy link
Member

I've removed windows for now: e8d426f.

Fell free to put it back.

@dimitrioskarvounaris
Copy link
Contributor

dimitrioskarvounaris commented Jun 5, 2020

@nunomaduro Branch "windows-gitbash" contains Pierstoval's as also my fixes, not "windows". #61 fixes everything and the tests pass (I run them on windows as also linux).

@nunomaduro
Copy link
Member

Closing in favour of #61.

@nunomaduro nunomaduro closed this Jun 5, 2020
@Pierstoval Pierstoval deleted the windows branch April 19, 2021 08:30
@dkarvounaris
Copy link

Dear colleagues, I am working on software engineering research and request your cooperation in answering this survey: https://forms.gle/h2D6cfjJM6rtSV456

It will only take you 10 minutes.

Excuse me, thank you very much for your help.

Is this some spam?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Builds failing on windows
5 participants