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

tests: always use real bcrypt lib #962

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Aug 28, 2023

  • remove dependency injection of bcrypt
  • remove mock-bcrypt.js
  • remove multiple paths to password.verify()/verifyPassword() and password.hash()/hashPassword()
  • cut CI build times from ~8.5 mins to ~2.5 mins

@matthew-white
Copy link
Member

How does bcrypt with a salt of 1 compare to the bcrypt mock in terms of speed? That is, how do times compare when testing locally (where BCRYPT=no previously)?

@alxndrsn
Copy link
Contributor Author

I've just run time make test locally, once for each:

master

real	3m20.993s
user	1m39.687s
sys	0m10.963s

this branch

real	3m21.863s
user	1m41.048s
sys	0m10.810s

@alxndrsn
Copy link
Contributor Author

alxndrsn commented Aug 28, 2023

On my machine, both bcrypt.hashSync() and bcrypt.validateSync() take about 1ms with a cost factor of 4:

hash

> bcrypt = require('bcrypt'); start = Date.now(); for(i=0; i<1000; ++i) bcrypt.hashSync('' + Math.random(), 1); Date.now() - start;
1125

validate

> bcrypt = require('bcrypt'); hashes = []; for(i=0; i<1000; ++i) { plaintext=''+Math.random(); hashes.push([plaintext, bcrypt.hashSync(plaintext, 1)])}; start = Date.now(); hashes.forEach(([plain, hash]) => bcrypt.compareSync(plain, hash)); Date.now() - start
1104

@sadiqkhoja
Copy link
Contributor

I like this change, and want this to be merged soon

@matthew-white
Copy link
Member

I haven't taken a close look, but no objection on my end! Maybe @alxndrsn should rebase, then @sadiqkhoja should review.

One thing that comes to mind is that this PR seems thematically related to #990 in that both use environment variables rather than dependency injection, which we've tended to use in the past. #990 is partly meant to facilitate testing of the CLI, for which we don't have dependency injection set up: see #937. I don't think there's an issue with a greater use of environment variables, but I thought I'd note the pattern.

Instead of mocking bcrypt in some test scenarios, decrease the cost factor to
the fastest possible value.
@alxndrsn alxndrsn force-pushed the use-bcrypt-in-tests-fast branch from 7c92c7e to d45d886 Compare November 3, 2023 12:31
@alxndrsn
Copy link
Contributor Author

alxndrsn commented Nov 3, 2023

I haven't taken a close look, but no objection on my end! Maybe @alxndrsn should rebase, then @sadiqkhoja should review.

This has been rebased 👍

Makefile Outdated Show resolved Hide resolved
@matthew-white matthew-white requested review from sadiqkhoja and matthew-white and removed request for sadiqkhoja November 7, 2023 16:27
@matthew-white
Copy link
Member

@sadiqkhoja, I know you're focused on web forms at the moment, but would you be up for reviewing this PR? I can also take a look if you don't have time.

@matthew-white matthew-white requested review from sadiqkhoja and removed request for matthew-white February 8, 2024 18:45
Copy link
Contributor

@sadiqkhoja sadiqkhoja left a comment

Choose a reason for hiding this comment

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

this is a nice enhancement

@alxndrsn alxndrsn merged commit ea188f4 into getodk:master Feb 14, 2024
1 check passed
@alxndrsn alxndrsn deleted the use-bcrypt-in-tests-fast branch February 14, 2024 11:23
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