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

Simplify custom tests #43

Merged
merged 1 commit into from
Nov 3, 2024
Merged

Simplify custom tests #43

merged 1 commit into from
Nov 3, 2024

Conversation

uuf6429
Copy link
Member

@uuf6429 uuf6429 commented Nov 3, 2024

The base TestCase from driver-testsuite has some disadvantages:

  • it tends to require some additional boilerplate for most driver-specific tests
  • it's a bit unclear/convoluted for simple test scenarios
  • in principle, tests requiring mink/getSession() are probably misplaced and should exist in the driver testsuite when you think about it

With that in mind, I came up with a simple base TestCase class that copies some of the functionality of the original one.
In my opinion, we should make helper traits out of the original one, which driver authors could use - but that's a bit off topic.

@uuf6429 uuf6429 requested a review from aik099 November 3, 2024 18:20
Copy link

codecov bot commented Nov 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.61%. Comparing base (9afb55f) to head (4e6615f).

Additional details and impacted files
@@             Coverage Diff              @@
##               main      #43      +/-   ##
============================================
+ Coverage     84.21%   84.61%   +0.40%     
  Complexity      194      194              
============================================
  Files             1        1              
  Lines           494      494              
============================================
+ Hits            416      418       +2     
+ Misses           78       76       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@aik099 aik099 left a comment

Choose a reason for hiding this comment

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

Now I've got the core idea behind this PR: if we want to test the driver, then we need to instantiate driver class and only call it.

Currently, in the driver test suite, we use NodeElement and Session to manipulate the driver, but that only slows things down and makes it harder to understand what driver calls are actually made. We're basically testing the Mink Driver using the Mink itself, which is a bad practice.

You can suggest a driver test suite rewrite idea at https://github.com/minkphp/driver-testsuite , but I'm not sure who will volunteer to rewrite a test suite, that already works.

@uuf6429 uuf6429 merged commit 61b12d0 into main Nov 3, 2024
30 of 32 checks passed
@uuf6429 uuf6429 deleted the chore/simplify-custom-tests branch November 3, 2024 20:01
@stof
Copy link
Member

stof commented Nov 4, 2024

For custom tests covering driver-specific configuration settings, using the driver directly makes sense (and is what we already do in a bunch of tests of other drivers).
However, for the shared tests, writing them as functional tests using the high-level API of Mink makes sense IMO

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

Successfully merging this pull request may close these issues.

3 participants