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

Improving reliability of get_current_url tests #398

Merged
merged 3 commits into from Sep 24, 2021
Merged

Conversation

pauarge
Copy link
Contributor

@pauarge pauarge commented Sep 24, 2021

Description

This PR improves the tests on the get_current_url function. Concretely, we're changing how we check the results, from assertStringStartsWith to the stricter assertEquals.

With this PR, we have tests to prove that if a query parameter, such as ?p=1 gets added to the URL, the function will correctly catch it. This closes an issue (linked below) that pointed to this function not working correctly.

Motivation and Context

#151

How Has This Been Tested?

Ran PHPUnit tests locally and in CI.

Types of changes

New feature (non-breaking change which adds functionality)

@pauarge pauarge changed the title Updating tests to be more reliable Improving reliability of get_current_url tests Sep 24, 2021
@pauarge pauarge added this to the 2.6.0 milestone Sep 24, 2021
@pauarge pauarge marked this pull request as ready for review September 24, 2021 09:13
@pauarge pauarge requested a review from a team as a code owner September 24, 2021 09:13
The focus of these tests is on the parsing issues related to the domain, port, and potential path, and this is represented in the data provider.

However, it's good to be able to confirm that going to the homepage, random URL and a post via it's ID, also work well for each of the combinations in the first parts of the URL.
@GaryJones
Copy link
Contributor

The focus of these tests is on the parsing issues related to the domain, port, and potential path, and this is represented in the data provider.

However, it's good to be able to confirm that going to the homepage, random URL and a post via its ID, also work well for each of the combinations in the first parts of the URL.

Since the setup is the same for each of the tests, we can just merge these three tests into one, and have it test the homepage, a specific post, and a random URL, and add the message attribute to the assertions to make it clear which part might have failed.

If you're happy with my changes @pauarge, then I can rebase into a single commit or it can be merged with a squash, to keep the history cleaner.

@pauarge
Copy link
Contributor Author

pauarge commented Sep 24, 2021

@GaryJones Your changes look good to me, they make the tests easier to read. It's a 👍 from my side.

@pauarge pauarge merged commit 63dfccd into develop Sep 24, 2021
@pauarge pauarge deleted the fix/get_current_url branch September 24, 2021 11:28
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.

None yet

2 participants