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

Setting some options to 0 doesn't work (it still uses defaults) #824

Closed
ollelindeman opened this issue Oct 4, 2023 · 2 comments · Fixed by #825 or #820
Closed

Setting some options to 0 doesn't work (it still uses defaults) #824

ollelindeman opened this issue Oct 4, 2023 · 2 comments · Fixed by #825 or #820
Labels

Comments

@ollelindeman
Copy link

Problem

I noticed when writing tests that setting some options to 0 is not working as I expected.

An example is errorThresholdPercentage, which I wanted to set to 0 in a test to make it fail immediately. But setting it to 0 will result in the default value 50 being used!

Is that the expected behaviour?


The reason why this happens is in how the defaults are set in the CircuitBreaker constructor.

this.options.errorThresholdPercentage =
      options.errorThresholdPercentage || 50;

https://github.com/nodeshift/opossum/blob/main/lib/circuit.js#L157C5-L158C46

Since 0 is considered false it will take the 50.

Similar behaviour is also present for the options resetTimeout, rollingCountTimeout, rollingCountBuckets.

@lholmquist lholmquist added the bug label Oct 4, 2023
@lholmquist
Copy link
Member

thanks for finding that!! we should probably add some tests to make sure we can set these values to 0

lholmquist added a commit to lholmquist/opossum that referenced this issue Oct 4, 2023
when setting things like resetTimeout and errorThresholdPercentage to 0, the default value was being used.

fixes nodeshift#824
lholmquist added a commit that referenced this issue Oct 5, 2023
when setting things like resetTimeout and errorThresholdPercentage to 0, the default value was being used.

fixes #824
@lholmquist
Copy link
Member

just released 8.1.2 on npm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants