Skip to content

[Tests] Streamline #52402

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
Merged
merged 1 commit into from
Dec 26, 2023
Merged

[Tests] Streamline #52402

merged 1 commit into from
Dec 26, 2023

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Nov 1, 2023 •
edited
Loading

Q A
Branch? 7.1
Bug fix? no
New feature? no
Deprecations? no
Issues --
License MIT

Follows

@OskarStark OskarStark self-assigned this Nov 1, 2023
@OskarStark OskarStark requested a review from xabbuh as a code owner November 1, 2023 08:15
@carsonbot carsonbot added this to the 6.4 milestone Nov 1, 2023
@OskarStark OskarStark requested a review from lyrixx as a code owner November 1, 2023 08:26
OskarStark
OskarStark commented Nov 1, 2023
View reviewed changes
src/Symfony/Component/Workflow/Tests/Validator/StateMachineValidatorTest.php Outdated

$this->expectException(InvalidDefinitionException::class);
$this->expectExceptionMessage('The state machine "foo" cannot store many places. But the definition has 2 initial places. Only one is supported.');

(new StateMachineValidator())->validate($definition, 'foo');

// the test ensures that the validation does not fail (i.e. it does not throw any exceptions)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

are the following lines still needed @lyrixx I don't understand that part 🤔

Copy link
Member

Choose a reason for hiding this comment

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

this makes no sense I agree, let's see what happens when removing the next lines

Copy link
Member

Choose a reason for hiding this comment

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

could be also the result of a bad merge ? how does this look like on lower branches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code looks the same until 4.4, I will provide a PR against 5.4 for this test and revert here

@OskarStark OskarStark requested a review from stof November 1, 2023 08:34
@OskarStark OskarStark force-pushed the feature/streamline-tests-6.4 branch from 7add3d7 to bac4e6c Compare November 1, 2023 13:23
@OskarStark OskarStark mentioned this pull request Nov 2, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Nov 2, 2023
@nicolas-grekas
minor #52421 [PasswordHasher] [Tests] Do not invoke methods with addi…
6765a43 
…tional arguments in tests (OskarStark)

This PR was merged into the 5.4 branch.

Discussion
----------

[PasswordHasher] [Tests] Do not invoke methods with additional arguments in tests

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | ---
| License       | MIT

Extracted from #52402

I think its worth to merge this in 5.4

Commits
-------

fdaefc4 [PasswordHasher][Tests] Do not invoke methods with additional arguments in tests
@OskarStark OskarStark force-pushed the feature/streamline-tests-6.4 branch from 27946e7 to 10b0f48 Compare December 20, 2023 23:11
@OskarStark
Copy link
Contributor Author

How easy would it be for you to revert all the added return types from providers?
They don't provide any benefit IMHO but make reviewing harder + will generate merge needless conflicts.

Done, good to go @nicolas-grekas

@OskarStark OskarStark mentioned this pull request Dec 20, 2023
Merged
fabpot added a commit that referenced this pull request Dec 21, 2023
@fabpot
minor #53166 [Workflow] Fix test (OskarStark)
100e5bc 
This PR was merged into the 5.4 branch.

Discussion
----------

[Workflow] Fix test

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Follows #52402 (comment)
| License       | MIT

Commits
-------

02fa2ec [Workflow] Fix test
@OskarStark OskarStark force-pushed the feature/streamline-tests-6.4 branch from 10b0f48 to ee89a98 Compare December 21, 2023 11:22
@xabbuh
Copy link
Member

xabbuh commented Dec 21, 2023

the failures look related

@OskarStark
Copy link
Contributor Author

the failures look related

fixed

@OskarStark OskarStark force-pushed the feature/streamline-tests-6.4 branch from e93ad50 to 9d64921 Compare December 22, 2023 08:30
@OskarStark
Copy link
Contributor Author

Rebased ✅

@OskarStark OskarStark mentioned this pull request Dec 23, 2023
Merged
@nicolas-grekas nicolas-grekas force-pushed the feature/streamline-tests-6.4 branch from 9d64921 to 33d71aa Compare December 26, 2023 14:02
@nicolas-grekas
Copy link
Member

Thank you @OskarStark.

@nicolas-grekas nicolas-grekas merged commit e6acb8c into symfony:7.1 Dec 26, 2023
fabpot added a commit that referenced this pull request Dec 29, 2023
@fabpot
minor #52481 [Tests] Streamline CompiledUrlGenerator tests (OskarSt…
895081d 
…ark)

This PR was merged into the 5.4 branch.

Discussion
----------

[Tests] Streamline `CompiledUrlGenerator` tests

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Follows #52402
| License       | MIT

weird, it looks like the expected exception is thrown in $this->generatorDumper->dump() 🤔 So the following code with the CompiledUrlGenerator is not needed, so maybe the test is not needed anymore or the test must be adjusted somehow to test the behavior of CompiledUrlgenerator....

Commits
-------

807d70e [Tests] Streamline CompiledUrlGenerator tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@derrabus derrabus derrabus left review comments

@xabbuh xabbuh xabbuh approved these changes

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

@stof stof Awaiting requested review from stof

@welcoMattic welcoMattic Awaiting requested review from welcoMattic welcoMattic is a code owner

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees

@OskarStark OskarStark

Labels
Status: Reviewed
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
6 participants
@OskarStark @nicolas-grekas @xabbuh @derrabus @chalasr @carsonbot