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

Move test web server startup/shutdown to the GitHub Actions workflow #22

Closed
aik099 opened this issue Mar 11, 2024 · 3 comments
Closed

Comments

@aik099
Copy link
Member

aik099 commented Mar 11, 2024

Currently, the https://github.com/minkphp/webdriver-classic-driver/blob/main/tests/bootstrap.php file automatically starts/stops the web server with test fixtures used during testing. This is an innovative approach compared to how other drivers would do this.

If we go away from the current approach (start web server inside a test suite), then all that stuff needs to be moved into the GitHub Actions workflow (as in other drivers).

Possible downsides to the current approach (I'm not saying it's a wrong approach in general):

  • Project contributors might already have a Web Server running locally and this project cloned in its DocumentRoot. In that scenario, they don't need another Web Server to be started and only need to map paths in the phpunit.xml.
  • The 8002 port might be already occupied on the contributor's machine, which would result in the inability to run tests.
  • The Symfony's Process component is being used, but it's not explicitly listed as a dev dependency in the composer and that means, that it's not guaranteed to be present as a dependency.
@uuf6429
Copy link
Member

uuf6429 commented Mar 11, 2024

Another downside:

  • developers cannot try pages in the test server without either running the server manually, or run tests with a debugger+breakpoint, very long sleep or similar.

On the positive side:

  • developers do not need to remember to start the web server
  • we can avoid having this step all over the place (contrib md, github actions)
  • in case of port conflicts, we could make it somehow allocate and use an unused arbitrary port

The Symfony's Process component is being used, but it's not explicitly listed as a dev dependency in the composer and that means, that it's not guaranteed to be present as a dependency.

That's a good point, I'll fix that. Done: #23

@aik099
Copy link
Member Author

aik099 commented Mar 11, 2024

in case of port conflicts, we could make it somehow allocate and use an unused arbitrary port

The default port can be specified through the environment variable (as we do with other test-related stuff) in the phpunit.xml.dist file.

@uuf6429
Copy link
Member

uuf6429 commented Nov 1, 2024

  • Project contributors might already have a Web Server running locally and this project cloned in its DocumentRoot.

Interestingly, the original process bound to that port would receive the requests. For some reason PHP doesn't complain that the port is already in use.

Worst case is that the original server's purpose was unrelated to these tests, so the tests start failing, confusing the user.

But that case would happen anyway with the old manual flow.

  • The 8002 port might be already occupied on the contributor's machine, which would result in the inability to run tests.

They'll need to change that in the config then. That also applies to other ports (e.g. defined in docker).

  • The Symfony's Process component is being used, but it's not explicitly listed as a dev dependency in the composer and that means, that it's not guaranteed to be present as a dependency.

Fixed in #23.

  • The default port can be specified through the environment variable (as we do with other test-related stuff) in the phpunit.xml.dist file.

Will be fixed #34 and documentation improved in #19.


A small reminder of why this was done: we should strive for the simplest way possible for contributors to start out. This is why we use tools like package managers, containerisation and so on.

With all that, I feel that this issue can be closed.

@uuf6429 uuf6429 closed this as completed Nov 1, 2024
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

No branches or pull requests

2 participants