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

Fixed unit test suite that was not running, added working browser test #11671

Merged
merged 2 commits into from
Sep 27, 2022

Conversation

chartjes
Copy link

@chartjes chartjes commented Aug 12, 2022

Description

I was asked by Alison to investigate why the unit and browser tests were not working.

Type of change

Please delete options that are not relevant.

  • [ x] Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Verified that existing unit test suite runs (still reporting errors but there are no environment or configuration-related errors, just failing tests)

Test Configuration:

  • PHP version: PHP 8.1.9
  • MySQL version: MariaDB from Docker-based install
  • Webserver version: Version in Docker-based install
  • OS version: MacOS 12.5

Checklist:

  • [x ] I have read the Contributing documentation available here: https://snipe-it.readme.io/docs/contributing-overview
  • [ x] I have formatted this PR according to the project guidelines: https://snipe-it.readme.io/docs/contributing-overview#pull-request-guidelines
  • [x ] My code follows the style guidelines of this project
  • [x ] I have performed a self-review of my own code
  • [x ] I have commented my code, particularly in hard-to-understand areas
  • [x ] I have made corresponding changes to the documentation
  • [x ] My changes generate no new warnings
  • [ x] I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@chartjes chartjes requested a review from snipe as a code owner August 12, 2022 18:08
@welcome
Copy link

welcome bot commented Aug 12, 2022

💖 Thanks for this pull request! 💖

We use semantic commit messages to streamline the release process and easily generate changelogs between versions. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix if it doesn't have one already.

Examples of commit messages with semantic prefixes:

  • Fixed #<issue number>: don't overwrite prevent_default if default wasn't prevented
  • Added #<issue number>: add checkout functionality to assets
  • Improved Asset Checkout: use new notification method for checkout

Things that will help get your PR across the finish line:

  • Document any user-facing changes you've made.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@chartjes chartjes changed the title Got unit and browser tests working, added documentation Fixed unit test suite that was not running, added working browser test Aug 12, 2022
@snipe
Copy link
Owner

snipe commented Aug 14, 2022

Hi chartjes - can you retarget this to the develop branch, per the dev docs?

@snipe
Copy link
Owner

snipe commented Aug 14, 2022

Hm, still getting the same errors as before...

✨snipe@chodeblossom✨ snipe-it  (chartjes-ch-fix-tests) $ php artisan dusk tests/Browser/LoginTest.php
PHPUnit 9.5.21 #StandWithUkraine

E                                                                   1 / 1 (100%)

Time: 00:00.398, Memory: 34.50 MB

There was 1 error:

1) Tests\Browser\LoginTest::testLoginPageLoadsAndUserCanLogin
Facebook\WebDriver\Exception\WebDriverCurlException: Curl error thrown for http POST to /session with params: {"capabilities":{"firstMatch":[{"browserName":"chrome","goog:chromeOptions":{"args":["--window-size=1920,1080","--disable-gpu","--headless"]}}]},"desiredCapabilities":{"browserName":"chrome","platform":"ANY","chromeOptions":{"args":["--window-size=1920,1080","--disable-gpu","--headless"]}}}

Failed to connect to 127.0.0.1 port 9515: Connection refused

/Users/snipe/Sites/snipe-it/snipe-it/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php:333
/Users/snipe/Sites/snipe-it/snipe-it/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:131
/Users/snipe/Sites/snipe-it/snipe-it/tests/DuskTestCase.php:46
/Users/snipe/Sites/snipe-it/snipe-it/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:219
/Users/snipe/Sites/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Support/helpers.php:234
/Users/snipe/Sites/snipe-it/snipe-it/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:220
/Users/snipe/Sites/snipe-it/snipe-it/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:98
/Users/snipe/Sites/snipe-it/snipe-it/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:66
/Users/snipe/Sites/snipe-it/snipe-it/tests/Browser/LoginTest.php:32

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

This seems to happen whether I use php artisan serve beforehand or not. (IIRC, I shouldn't have to do that to run dusk.)

If it's any clue, I use MAMP Pro for local development. I'll use pretty much anything except for Docker tho.

@snipe
Copy link
Owner

snipe commented Aug 14, 2022

It also looks like it's not migrating for phpunit automatically. php artisan migrate --env=testing shows a bunch of migrations, but then it chokes somewhere on one of the migrations (adding primary key to password resets, which is required for later versions of MySQL), and then I get:

 • Tests\Unit\StatuslabelTest > lost statuslabel add
   Illuminate\Database\QueryException

  SQLSTATE[HY000]: General error: 1 no such table: status_labels (SQL: select count(*) as aggregate from "status_labels" where "name" = Lost/Stolen and "deleted_at" is null and "id" != 0)

  at vendor/laravel/framework/src/Illuminate/Database/Connection.php:712
    708▕         // If an exception occurs when attempting to run a query, we'll format the error
    709▕         // message to include the bindings with SQL, which will make this exception a
    710▕         // lot more helpful to the developer instead of just the database's errors.
    711▕         catch (Exception $e) {
  ➜ 712▕             throw new QueryException(
    713▕                 $query, $this->prepareBindings($bindings), $e
    714▕             );
    715▕         }
    716▕     }

      +11 vendor frames
  12  app/Providers/ValidationServiceProvider.php:51
      Illuminate\Database\Query\Builder::count()

      +17 vendor frames
  30  tests/Unit/StatuslabelTest.php:49
      Illuminate\Database\Eloquent\Factories\Factory::create()

@chartjes chartjes changed the base branch from master to develop August 19, 2022 15:41
@chartjes
Copy link
Author

If it's any clue, I use MAMP Pro for local development. I'll use pretty much anything except for Docker tho.

Ah...I will download MAMP Pro and take a look. It might be something as simple as adding details to the documentation about what port Dusk is running on.

@snipe
Copy link
Owner

snipe commented Aug 19, 2022

I mean, it shouldn’t matter tho? Whole point is that tests should be runnable from kind of anywhere, especially with something like Dusk, that promises to make all of this easier.

@chartjes
Copy link
Author

Dusk relies on Chromedriver to be the headless browser for the tests. There is a port that Chromedriver is using to accept connections and if in the various configuration files it is using a different one from what is expected, then it would be a good idea to make sure we force Chromedriver to always use the same port.

I don't think MAMP vs. Docker really matters. For Dusk, it's more about "have we provided configuration settings to make sure everyone has the same experience."

@snipe
Copy link
Owner

snipe commented Aug 19, 2022

100% yes - I did make sure I had chrome drivers set up, so I still don’t get why the extra steps. (This is confusion on my understanding of how Dusk is supposed to work, not on what you’ve done here.) It’s also possible Dusk is the wrong choice and we should go back to Codeception. Codeception was brittle, but mostly worked (with enough massaging.)

@chartjes
Copy link
Author

chartjes commented Aug 19, 2022 via email

@wewhite
Copy link
Contributor

wewhite commented Aug 19, 2022

I have it running.
Set tests/DuskTestCase.php: $_ENV['DUSK_DRIVER_URL'] ?? 'http://localhost:9515',
Set .env: APP_URL=http://127.0.0.1:8000

Open two shell windows

user1@computer:~/Programming/snipe-it-dev % php artisan serve
Starting Laravel development server: http://127.0.0.1:8000
[Fri Aug 19 14:32:46 2022] PHP 8.1.9 Development Server (http://127.0.0.1:8000) started
user2@computer:~/Programming/snipe-it-dev % php artisan dusk
PHPUnit 9.5.21 #StandWithUkraine

...                                                                 3 / 3 (100%)

Time: 00:04.034, Memory: 36.00 MB

OK (3 tests, 4 assertions)

@wewhite
Copy link
Contributor

wewhite commented Aug 19, 2022

user2@computer:~/Programming/snipe-it-dev %  php artisan dusk tests/Browser/LoginTest.php
PHPUnit 9.5.21 #StandWithUkraine

..                                                                  2 / 2 (100%)

Time: 00:03.052, Memory: 34.00 MB

OK (2 tests, 3 assertions)

@snipe
Copy link
Owner

snipe commented Aug 22, 2022

@chartjes have you had any luck with @wewhite's suggestions?

@wewhite
Copy link
Contributor

wewhite commented Aug 23, 2022

You have to have the .env, server and client configured correctly.
Port 8000 is used for the server.
Port 9515 is used for the client.

php       8995         vi1154    6u  IPv4 0x100033cddf341f1b      0t0    TCP 127.0.0.1:8000 (LISTEN)
chromedri 9007         vi1154    8u  IPv4 0x100033cdd6c2c9eb      0t0    TCP 127.0.0.1:9515 (LISTEN)
chromedri 9007         vi1154    9u  IPv6 0x100033cdd451890b      0t0    TCP [::1]:9515 (LISTEN)

Here are some commands I have used to diagnose:

// start server on specified host with verbose output
php artisan serve --host="127.0.0.1:8000" -vvv

// start server using specific .env file
php artisan serve --env=.env.dusk.local

// start client opening browser window with verbose output.
php artisan dusk --browse -vvv

@snipe
Copy link
Owner

snipe commented Aug 23, 2022

I'm not seeing anything in the docs about having to run php artisan serve outside of CI environments though. https://laravel.com/docs/8.x/dusk#running-tests

Laravel is a PHP web application framework with expressive, elegant syntax. We’ve already laid the foundation — freeing you to create without sweating the small things.

@wewhite
Copy link
Contributor

wewhite commented Aug 23, 2022

In the docs, "By default, Dusk will automatically attempt to start ChromeDriver. If this does not work for your particular system, you may manually start ChromeDriver before running the dusk command."

With your original error Failed to connect to 127.0.0.1 port 9515: Connection refused, this is because the chromedriver did not start automatically.
You can do the following, it auto starts to 9515.
Add the WEBDRIVER_CHROME_DRIVER bin folder to your path:

user@development:~/Programming/snipe-it % export WEBDRIVER_CHROME_DRIVER=~/Programming/snipe-it/vendor/laravel/dusk/bin/
user@development:~/Programming/snipe-it % export PATH=$PATH:$WEBDRIVER_CHROME_DRIVER

And start it manually:

user@development:~/Programming/snipe-it % chromedriver-mac-arm
Starting ChromeDriver 104.0.5112.79 (3cf3e8c8a07d104b9e1260c910efb8f383285dc5-refs/branch-heads/5112@{#1307}) on port 9515
Only local connections are allowed.
Please see https://chromedriver.chromium.org/security-considerations for suggestions on keeping ChromeDriver safe.
ChromeDriver was started successfully.

Then from another window, run php artisan dusk --verbose --browse.

Sorry, I'm throwing in a lot of information, on dump mode.

I start the server with php artisan serve, since it makes sense to me to run tests against same code that the server is running. I think by default, it will start an instance serving out on port 8000.
The headless chrome browser/driver starts and listens on port 9515

Looks like the php artisan dusk command uses the .env.dusk config file by default. Adjust as necessary.

@wewhite
Copy link
Contributor

wewhite commented Aug 23, 2022

Here is an interesting read, of all the different ways people have gotten php artisan dusk to work:
https://stackoverflow.com/questions/63679593/laravel-dusk-facebook-webdriver-exception-unknownerrorexception-unknown-error

I read all the comments, starting to understand dusk a little more.

Stack Overflow
Running php artisan dusk get the error: Facebook\WebDriver\Exception\UnknownErrorException: unknown error: net::ERR_CONNECTION_REFUSED (Session info: headless chrome=85.0.4183.83)

Versions:

OS:

@chartjes
Copy link
Author

I have managed to get everything up-and-running at my end, thank you to @wewhite for digging into it some more. I have updated the documentation to reflect what was discovered. I'm 99.999% sure I am not using the correct branch for this PR. Please take a look so it can be closed out.

@snipe snipe merged commit 8d3fe42 into snipe:develop Sep 27, 2022
@welcome
Copy link

welcome bot commented Sep 27, 2022

Congrats on merging your first pull request! 🎉🎉🎉

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

Successfully merging this pull request may close these issues.

3 participants