8000 [Testing] Document improving test speed by reducing password encoder "work factor" by kbond · Pull Request #13599 · symfony/symfony-docs · GitHub
[go: up one dir, main page]
Skip to content

[Testing] Document improving test speed by reducing password encoder "work factor" #13599

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
Jan 8, 2021
Merged

[Testing] Document improving test speed by reducing password encoder "work factor" #13599

merged 1 commit into from
Jan 8, 2021

Conversation

kbond
Copy link
Member
@kbond kbond commented Apr 27, 2020 •
edited
Loading

I also reduced my test suite run time by pre-encoding passwords for my test fixtures. I'm not sure if we should document this.

@kbond kbond force-pushed the test-encoder-config branch from 934d416 to 1dddc33 Compare April 27, 2020 20:24
@javiereguiluz
Copy link
Member

Thanks for this contribution.

Question to @symfony/team-symfony-docs I'm 99.99% sure that we have or had something like this in the docs. Maybe in the "Best Practices"? But I can't find it. Can you remember? Did we remove this from docs for some reason? Thanks.

@xabbuh
Copy link
Member
xabbuh commented May 12, 2020

I thought we had something about the plaintext password encoder, but I couldn't find anything. What do you think if we change the example here to make use of the plaintext encoder?

@kbond
Copy link
Member Author
kbond commented May 12, 2020 •
edited
Loading

A few arguments against plaintext:

  1. In my test suite, switching to the plaintext encoder breaks a few tests related to password migration (I check to see if legacy passwords are properly migrated on login).
  2. There was virtually no performance gain using plaintext vs sodium (with the lowest cost).
  3. I like to have my test environment as close as possible to production

That all being said, the plaintext config would be simpler. I'm fine switching if that's what you think would be best.

wouterj
wouterj approved these changes Nov 5, 2020
View reviewed changes
Copy link
Member
@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

I think this is great.

I also think we documented something like this already, but I think we must have lost it somewhere while moving things around (as I also can't find it anymore).

OskarStark
OskarStark reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor
@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Very nice, could you please add the comments to xml and php examples too? Thank you 👌🏻

@kbond kbond force-pushed the test-encoder-config branch from 1dddc33 to 6c3e356 Compare November 20, 2020 13:08
kbond
kbond commented Nov 20, 2020
View reviewed changes
@kbond
Copy link
Member Author
kbond commented Nov 20, 2020 •
edited
Loading

please add the comments to xml and php examples

Done. but see my issue with the xml comments.

@kbond kbond force-pushed the test-encoder-config branch 2 times, most recently from c6678b3 to 12ed6ad Compare November 20, 2020 14:34
@javiereguiluz javiereguiluz added this to the 4.4 milestone Jan 8, 2021
@javiereguiluz javiereguiluz changed the base branch from 5.0 to 4.4 January 8, 2021 14:27
@javiereguiluz javiereguiluz merged commit 4baf40c into symfony:4.4 Jan 8, 2021
@javiereguiluz
Copy link
Member

Thanks a lot Kevin! This was finally merged in 4.4 and up.

@kbond kbond deleted the test-encoder-config branch January 8, 2021 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj approved these changes

@OskarStark OskarStark OskarStark left review comments

@HeahDude HeahDude Awaiting requested review from HeahDude

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Status: Needs Review Testing
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@kbond @javiereguiluz @xabbuh @wouterj @OskarStark @carsonbot
0