mysociety / fixmystreet

This is mySociety's popular map-based reporting platform: easy to install in new countries and regions
http://fixmystreet.org/
Other
507 stars 235 forks source link

Mark tests to skip if optional packages they use are not installed #3340

Open lancew opened 3 years ago

lancew commented 3 years ago

When running the tests inside the development VM, there are four failing test files. When running tests via Github actions all test pass.

To Reproduce Steps to reproduce the behavior:

  1. vagrant up - Linux host, virtualbox provider
  2. vagrant ssh
  3. cd fixmystreet
  4. ./script/test

TAP output:

Test Summary Report
-------------------
t/app/controller/admin/templates.t        (Wstat: 256 Tests: 25 Failed: 1)
  Failed test:  24
  Non-zero exit status: 1
t/app/controller/waste.t                  (Wstat: 65280 Tests: 10 Failed: 3)
  Failed tests:  8-10
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output
t/cobrand/bromley.t                       (Wstat: 65280 Tests: 28 Failed: 6)
  Failed tests:  22-24, 26-28
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output
t/cobrand/tfl.t                           (Wstat: 65280 Tests: 21 Failed: 2)
  Failed tests:  20-21
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output
Files=166, Tests=2826, 588 wallclock secs ( 0.42 usr  0.89 sys + 324.57 cusr 228.60 csys = 554.48 CPU)
Result: FAIL

Any assistance working out why these tests pass on GitHub actions CI and not when run inside the VMM I'd appreciate it. Perhaps I have missed a step?

lancew commented 3 years ago

This PR has passing tests, for example. https://github.com/mysociety/fixmystreet/pull/3339

dracos commented 3 years ago

From a quick look, I guess the templates.t and tfl.t failures are because they are testing the internal_ips feature which the test is assuming to be 127.0.0.1, but presumably it is not actually that inside the VM somehow. [edit: was Net::Subnet missing]

Not sure about the other two (though the Bromley failures are to do with waste tests, so presumably they are somehow linked) - can you run those tests alone (e.g. script/test t/app/controller/waste.t ) so that it gives you a fuller output of why they have failed?

lancew commented 3 years ago

Hi, thanks for the reply. I have put the results of running the waste.t in a Gist: https://gist.github.com/lancew/05b06a7c4b49e349a33d34f4cfee1a58

Interestingly SOAP::Lite not installed, but I have since run script/update and say the module install and same result. I am not familiar enough with codebase, does run-tests use carton?

Hope the gist helps.

dracos commented 3 years ago

Ah, yes, the tests do require SOAP::Lite to be installed, but it's not installed by default as it's not needed for the base code to operate. script/update won't install it, as that (ends up) runs vendor/bin/carton install --deployment --without uk --without zurich --without kiitc. If you manually ran vendor/bin/carton install --deployment --without zurich --without kiitc (so the UK packages are installed), then those waste tests should hopefully pass. The GitHub Actions script installs all carton packages. We should set those tests to skip if SOAP::Lite is not installed.

lancew commented 3 years ago

Brilliant, thanks. Shall do that and feedback where that gets me. :-)

lancew commented 3 years ago

Ran vendor/bin/carton install --deployment --without zurich --without kiitc as suggested.

Then tested both waste.t and simply running script/test

All tests successful. Files=166, Tests=2893, 606 wallclock secs ( 0.42 usr 0.94 sys + 330.12 cusr 241.66 csys = 573.14 CPU) Result: PASS

dracos commented 3 years ago

Glad to hear it - it was actually the lack of Net::Subnet (also installed by the UK packages) causing the templates/TfL to fail. I'll leave this renamed as a hint to get the relevant tests to skip if the required packages aren't present.

lancew commented 3 years ago

Thanks @dracos, would it be OK to "skip" on entire file level, or is there a more targetted approach you'd prefer?

I.e. I have tested an approach like: https://github.com/mysociety/fixmystreet/blob/f32e0900dd8c0a97c2b2214c973394e1fda2cb0e/t/app/03podcoverage.t doing adding somewhere near the top:

eval "use SOAP::Lite"; plan skip_all => 'SOAP::Lite required' if $@; (or Net::Subnet)

Maybe I could add in documentation somewhere the vendor/bin/carton install --deployment --without zurich --without kiitc step, though you might be better placed to describe the why. :)