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 for result validation; Error in usage example #7

Closed
wants to merge 5 commits into from
Closed

Fix for result validation; Error in usage example #7

wants to merge 5 commits into from

Conversation

J-C-Q
Copy link
Contributor

@J-C-Q J-C-Q commented Feb 19, 2024

When running the usage example from the docs, there is an error when downloading the result.

Fixed it by adding meas_level and curc_id keys to the ExpResult schema.

Copy link
Member

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

status::String
success::Bool
meas_level::Int
circ_id::Int
time_taken::Float64
Copy link
Member

Choose a reason for hiding this comment

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

just to understand the change better cuz there are a lot format changes, is this the main update of the result schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, correct. Sorry for the formatting...

Copy link
Member

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! just need to wait for CI pass.

@Roger-luo
Copy link
Member

It seems the records are too old, causing CI to fail. Could you also update the records via https://github.com/JuliaTesting/BrokenRecord.jl ?

@J-C-Q
Copy link
Contributor Author

J-C-Q commented Feb 19, 2024

I want to try, but I'm not familiar with that.

So basically, I need to update the test/records/result.json with the content of the result.json I get from the usage example job, correct? However, they are very different, I'm guessing it's just the "body"?

@J-C-Q
Copy link
Contributor Author

J-C-Q commented Feb 19, 2024

Or do I have to delete the file to let it generate again?

Seems like I'm not authorized. But maybe the CI bot is?

@Roger-luo
Copy link
Member

Yes, first replace the key with your own, and delete all the old files, the BrokenRecord should generate the new files for you, replace the key with the fake one.

@J-C-Q
Copy link
Contributor Author

J-C-Q commented Feb 22, 2024

While trying to get this to work (I’m struggling a bit, maybe I need to ask some more questions later...), I noticed that the test for the menu doesn't work because it requires stdin to be a TTY (but it is an IOStream during the test). How do you say we proceed with that?

@Roger-luo
Copy link
Member

I noticed that the test for the menu doesn't work because it requires stdin to be a TTY (but it is an IOStream during the test). How do you say we proceed with that?

Hmm, that's strange I wrote it the same way how stdlib test it. But I think we can mark this @test_broken first in this PR since it's irrelevant

@J-C-Q
Copy link
Contributor Author

J-C-Q commented Feb 22, 2024

Alright.
So for the records, I noticed that the test account has different properties than my account, e.g., the URLs. My account doesn't have the -qcon. This way I can't just replace the submit-, status- and result.json since the request will be to a different URL during the playback with the test account. Should I create a new test account to also regenerate account.json etc., since I would rather not use my data for the account info tests?

@Roger-luo
Copy link
Member

So for the records, I noticed that the test account has different properties than my account, e.g., the URLs. My account doesn't have the -qcon. This way I can't just replace the submit-, status- and result.json since the request will be to a different URL during the playback with the test account. Should I create a new test account to also regenerate account.json etc., since I would rather not use my data for the account info tests?

You don't need to create an account, that test account is fake as well. You just need to generate the record and then replace the account info in the test manually using grep. So that there is no valid credentials created in the CI

@J-C-Q
Copy link
Contributor Author

J-C-Q commented Feb 22, 2024

Thank you for the help and your time!

I created a new pull request that should work. #8

@Roger-luo
Copy link
Member

closed by #8

@Roger-luo Roger-luo closed this Feb 23, 2024
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.

2 participants