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

fix(report): align printResult to output the correct total requests #380

Merged
merged 3 commits into from
Jul 7, 2021

Conversation

elirandav
Copy link
Contributor

Closes #228

@mcollina
Copy link
Owner

mcollina commented Jul 6, 2021

I'm not sure this is the correct fix, as the additional requests are done. This PR is just hiding them.

…use result.requests.sent instead of opts.amount

Closes mcollina#228
@elirandav
Copy link
Contributor Author

I'm not sure this is the correct fix, as the additional requests are done. This PR is just hiding them.

correct @mcollina .
we need to use the result and not opts. I aligned the PR.

In the scenario I am fixing, all the requests were sent to the tested server, only the output was wrong: 10 requests instead of 20 requests. I verified it with my local server - it receives all 20 requests.

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Lgtm

}
}
const opts = {
outputStream: {
Copy link
Owner

Choose a reason for hiding this comment

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

why is this object needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the test, I am simulating the bug in method printResult, which gets two parameters as input: result and opts.

so to gets the same behaviour while executing node autocannon..., I took the two objects value while debugging and used them as input for simulating the bug in the test.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this entire opts object is needed, could you remove the properties that are not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right. I removed the non-mandatory input. it seems throughput is mandatory with all its pX properties.

…removing input which is not mandatory for the test

Closes mcollina#228
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 455557e into mcollina:master Jul 7, 2021
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.

--amount is not precise with high concurrency
2 participants