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.

Sign up for GitHub

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

Jump to bottom

Refactoring tests #25420

Merged
merged 1 commit into from
Dec 12, 2017
Merged

Refactoring tests #25420

merged 1 commit into from
Dec 12, 2017

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Dec 10, 2017 •
edited

Q A
Branch? 2.7
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I've refactored some tests, using:

  • assertCount instead of count function;
  • assertArrayHasKey, assertArrayNotHasKey, assertObjectHasAttribute and assertObjectNotHasAttribute instead of isset function;
  • assertContains instead of in_array function;
  • assertSame and assertNotSame instead os strict comparisons ===;
  • assertNotFalse instead of strict comparisons !== with false keyword;
  • assertGreaterThan, assertLessThan and assertLessThanOrEqual for mathematical comparisons;

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Dec 11, 2017
@stof
Copy link
Member

stof commented Dec 11, 2017

such changes should be done in 2.7 first (and then additional PRs for 2.8, 3.3, 3.4, 4.0 and master with the necessary additional changes after things have been propagated each time).
Making our test diverge between master and older branches is making maintenance harder (as future changes to these tests would then create conflicts when propagating them to master)

@carusogabriel
Copy link
Contributor Author

carusogabriel commented Dec 11, 2017 •
edited

@stof @nicolas-grekas @OskarStark Should I replicate this PR to each branch? Isn't there a better way?

@fabpot
Copy link
Member

fabpot commented Dec 11, 2017

@carusogabriel Doing one PR on 2.7 is enough for now.

@carusogabriel
Copy link
Contributor Author

@fabpot Gonna work on that!

@carusogabriel carusogabriel changed the base branch from master to 2.7 December 11, 2017 21:56
@carusogabriel
Copy link
Contributor Author

Done! I've also added:

  • assertStringEqualsFile when comparing a string and a file;
  • assertFileEquals when comparing two files.

@nicolas-grekas
Copy link
Member

Thank you @carusogabriel.

@nicolas-grekas nicolas-grekas merged commit 567e0ab into symfony:2.7 Dec 12, 2017
nicolas-grekas added a commit that referenced this pull request Dec 12, 2017
@nicolas-grekas
minor #25420 Refactoring tests (carusogabriel)
e77545a 
This PR was merged into the 2.7 branch.

Discussion
----------

Refactoring tests

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

I've refactored some tests, using:
- `assertCount` instead of `count` function;
- `assertArrayHasKey`, `assertArrayNotHasKey`, `assertObjectHasAttribute` and `assertObjectNotHasAttribute` instead of `isset` function;
- `assertContains` instead of `in_array` function;
- `assertSame` and `assertNotSame` instead os strict comparisons `===`;
- `assertNotFalse` instead of strict comparisons `!==` with `false` keyword;
- `assertGreaterThan`, `assertLessThan` and `assertLessThanOrEqual` for mathematical comparisons;

Commits
-------

567e0ab Refactoring tests.
@carusogabriel carusogabriel deleted the refactoring-tests branch December 12, 2017 11:08
@chalasr chalasr mentioned this pull request Dec 12, 2017
Merged
nicolas-grekas added a commit that referenced this pull request Dec 12, 2017
@nicolas-grekas
minor #25463 [HttpFoundation] Fix tests (chalasr)
b49998e 
This PR was merged into the 3.3 branch.

Discussion
----------

[HttpFoundation] Fix tests

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | not yet
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Remains filesystem ones due to #25420, going to have a look in this PR in the next hour if nobody does before.

Commits
-------

ef6adb8 Fix tests
@carusogabriel carusogabriel mentioned this pull request Dec 19, 2017
Merged
nicolas-grekas added a commit that referenced this pull request Dec 20, 2017
@nicolas-grekas
minor #25553 Add php_unit_dedicate_assert to PHPCS (carusogabriel)
f6fc785 
This PR was merged into the 2.7 branch.

Discussion
----------

Add php_unit_dedicate_assert to PHPCS

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25420
| License       | MIT
| Doc PR        | -

Forgot to add this in #25420 😅

Commits
-------

e913b68 Add php_unit_dedicate_assert to PHPCS
@carusogabriel carusogabriel mentioned this pull request Jan 2, 2018
Merged
This was referenced Jan 23, 2018
Merged
Merged
fabpot added a commit that referenced this pull request Jan 29, 2018
@fabpot
minor #25907 Improve assertions (carusogabriel)
d007bbd 
This PR was merged into the 2.7 branch.

Discussion
----------

Improve assertions

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Following #25420 before start other branches.

Btw, the other branches are 2.8, 3.3, 3.4, 4.0 and master?

Commits
-------

3d90a22 Improve assertions
fabpot added a commit that referenced this pull request Jan 29, 2018
@fabpot
minor #25908 Improve assertions (carusogabriel)
b99ca76 
This PR was merged into the 2.8 branch.

Discussion
----------

Improve assertions

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Following #25420 in `2.8` branch.

Commits
-------

4683f6d Improve assertions
@carusogabriel carusogabriel mentioned this pull request Feb 1, 2018
Merged
nicolas-grekas added a commit that referenced this pull request Feb 4, 2018
@nicolas-grekas
minor #26002 Improve assertions (carusogabriel)
9a3aa07 
This PR was merged into the 3.4 branch.

Discussion
----------

Improve assertions

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Following #25420 in `3.4` branch.

Commits
-------

829f59d Improve assertions
@carusogabriel carusogabriel mentioned this pull request Feb 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark approved these changes

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@carusogabriel @stof @fabpot @nicolas-grekas @OskarStark @carsonbot