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

myjsonrpc: Go back to incremental parsing #1248

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

perlpunk
Copy link
Contributor

@perlpunk perlpunk commented Oct 29, 2019

The strategy to look for the next newline was too slow.

The time for read_json() increased to about 4-5 times with the changes introduced in #1230

https://progress.opensuse.org/issues/58823 "os-autoinst is too slow pressing F2 causing ARM tests to fail in "boot_to_desktop"

The write_json seemed to fail sometimes because the read took too long, resulting in test failures.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Please also describe in your git commit message in more details what is fixed without relying solely on the ticket.

The strategy to look for the next newline was too slow.

The time for read_json() increased to about 4-5 times with the changes
introduced in os-autoinst#1230 ec50b87

https://progress.opensuse.org/issues/58823
"os-autoinst is too slow pressing F2 causing ARM tests to fail in
"boot_to_desktop"

The write_json seemed to fail sometimes because the read took too long,
resulting in test failures.
@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #1248 into master will increase coverage by 1.51%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1248      +/-   ##
==========================================
+ Coverage   38.39%   39.91%   +1.51%     
==========================================
  Files          40       40              
  Lines        4956     4948       -8     
  Branches      846      844       -2     
==========================================
+ Hits         1903     1975      +72     
+ Misses       2712     2630      -82     
- Partials      341      343       +2
Impacted Files Coverage Δ
myjsonrpc.pm 70.68% <100%> (-2.04%) ⬇️
isotovideo 72.97% <0%> (+0.38%) ⬆️
needle.pm 49.72% <0%> (+0.55%) ⬆️
backend/qemu.pm 32.78% <0%> (+1.28%) ⬆️
consoles/VNC.pm 44.06% <0%> (+1.51%) ⬆️
consoles/console.pm 52.17% <0%> (+2.17%) ⬆️
consoles/vnc_base.pm 42.7% <0%> (+3.12%) ⬆️
consoles/network_console.pm 91.66% <0%> (+8.33%) ⬆️
backend/baseclass.pm 58.21% <0%> (+8.61%) ⬆️
consoles/sshIucvconn.pm 36% <0%> (+16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 112f346...6ceed99. Read the comment docs.

@perlpunk
Copy link
Contributor Author

The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Please also describe in your git commit message in more details what is fixed without relying solely on the ticket.

I added info to the commit message. Not sure what I should add. Maybe @Martchus has suggestions?

@perlpunk perlpunk requested a review from okurz October 29, 2019 17:29
@coolo coolo merged commit 327be13 into os-autoinst:master Oct 29, 2019
@okurz
Copy link
Member

okurz commented Oct 30, 2019

Thanks for the quick merge and the added details in the commit message

@perlpunk perlpunk deleted the myjsonrpc-incr branch November 5, 2019 11:16
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