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

Update benchmark journey to use complex scenarios #136

Merged
merged 26 commits into from
Oct 27, 2023

Conversation

Yuyuutsu
Copy link
Contributor

@Yuyuutsu Yuyuutsu commented Sep 12, 2023

What is the context of this PR?

Updated the business benchmark json and created a happy and unhappy json path.

This PR is linked with the PR in eq-questionnaire-runner.

Trello Card

How to review

Check if schema addresses all the new suggested scenarios in the Trello Card
Test both benchmarks locally and via a deployed staging
The tag should be update-benchmark-journey-to-test-complex-scenarios

@Yuyuutsu
Copy link
Contributor Author

Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

The existing request file test_benchmark_business will no longer work so should probably be removed. There are also some request files relating to the PDF download tests that will be broken by updating the new schema, so not sure if we want to keep these?

@Yuyuutsu
Copy link
Contributor Author

The existing request file test_benchmark_business will no longer work so should probably be removed. There are also some request files relating to the PDF download tests that will be broken by updating the new schema, so not sure if we want to keep these?

I shall remove the test_benchmark_business and update the PDF download tests we have 👍

@MebinAbraham
Copy link
Contributor

The existing request file test_benchmark_business will no longer work so should probably be removed. There are also some request files relating to the PDF download tests that will be broken by updating the new schema, so not sure if we want to keep these?

We should keep any existing feature tests such as testing PDF download.

@petechd
Copy link
Contributor

petechd commented Oct 6, 2023

Shouldn't we restore schemas that test pdf kit and weasy?

@Yuyuutsu
Copy link
Contributor Author

Yuyuutsu commented Oct 6, 2023

Shouldn't we restore schemas that test pdf kit and weasy?

I have now pushed up PDF kit optimised 👍 . But weasy was removed because it was never meant to be in main.

@MebinAbraham
Copy link
Contributor

MebinAbraham commented Oct 6, 2023

Shouldn't we restore schemas that test pdf kit and weasy?

I have now pushed up PDF kit optimised 👍 . But weasy was removed because it was never meant to be in main.

Both are not needed.

You can exclude PDF endpoint as they will skew the latency as PDF is significantly slower than the rest of the app.
A PDF one can be added when we do PDF specific testing.

@Yuyuutsu
Copy link
Contributor Author

Yuyuutsu commented Oct 6, 2023

Shouldn't we restore schemas that test pdf kit and weasy?

I have now pushed up PDF kit optimised 👍 . But weasy was removed because it was never meant to be in main.

Both are not needed.

You can exclude PDF endpoint as they will skew the latency as PDF is significantly slower than the rest of the app. A PDF one can be added when we do PDF specific testing.

Removed both 🫡

Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

A PR will need to be raised for the pipelines repo if we are changing the name of the test_benchmark_buisness requests file, as that is the one referenced in the daily benchmark vars file

Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

Possibly not relevant to this PR, but since we're updating the requests files here, the feedback requests json doesn't seem to work

Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

The updated journeys/requests files are all working, but was the plan to update the benchmark scripts so that locust runs both the unhappy and happy paths?

@MebinAbraham
Copy link
Contributor

The updated journeys/requests files are all working, but was the plan to update the benchmark scripts so that locust runs both the unhappy and happy paths?

Not yet, we should update to use happy path for daily test. We will do another card to do a comparison benchmark of the two. Then look to run unhappy only/both.

@Yuyuutsu
Copy link
Contributor Author

Possibly not relevant to this PR, but since we're updating the requests files here, the feedback requests json doesn't seem to work

the schema changed but we forgot to change the feedback har file. I have a fix ready to push up if we want to push it up in this PR or in another PR

@berroar
Copy link
Contributor

berroar commented Oct 26, 2023

Possibly not relevant to this PR, but since we're updating the requests files here, the feedback requests json doesn't seem to work

the schema changed but we forgot to change the feedback har file. I have a fix ready to push up if we want to push it up in this PR or in another PR

I'd create another PR for it 👍

@Yuyuutsu Yuyuutsu merged commit 4dfa977 into main Oct 27, 2023
2 checks passed
@Yuyuutsu Yuyuutsu deleted the update-benchmark-journey-to-test-complex-scenarios branch October 27, 2023 10:58
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.

5 participants