ic icon indicating copy to clipboard operation
ic copied to clipboard
verified publisher icon dfinity

fix(icos): Several testing and performance improvements to SetupOS

Open DFINITYManu opened this issue 1 year ago • 1 comments

  • Improving performance of HostOS and GuestOS installation within tests and in prod.
  • Document how to start SetupOS interactively.
  • Provide facilities for interactive testing unobstructed by automated setup process.
  • Change methodology of SetupOS testing to speed up tests.

This PR rides atop this other one: https://github.com/dfinity/ic/pull/1701

DFINITYManu avatar Oct 16 '24 12:10 DFINITYManu

Thank you for doing this Manuel!

andrewbattat avatar Oct 18 '24 15:10 andrewbattat

Alright! I think we're ready to go. What is missing from this PR?

DFINITYManu avatar Oct 28 '24 17:10 DFINITYManu

Alright! We're ready for review. The interactivetesting branch was merged here, as requested in the other branch.

DFINITYManu avatar Nov 05 '24 12:11 DFINITYManu

Removing cross-chain team since the label was due to the PR being out-of-date with master.

gregorydemay avatar Nov 05 '24 12:11 gregorydemay

Alright! We're ready for review. The interactivetesting branch was merged here, as requested in the other branch.

Could you please still address the remaining comments in that PR so that I don't have to add them here again?

frankdavid avatar Nov 05 '24 12:11 frankdavid

Alright! We're ready for review. The interactivetesting branch was merged here, as requested in the other branch.

Could you please still address the remaining comments in that PR so that I don't have to add them here again?

Yes.

I see this one:

https://github.com/dfinity/ic/pull/2115/#discussion_r1829241222

I'm not convinced about these suggested efficiency improvements of using String over Vec since the smallest string is going to be more than 4 bytes anyway (remember it's not just the actual string stored but also the length) and we're going character by character.

I'm working on figuring out exactly how to implement what you suggested without getting errors, nonetheless.

EDIT: I've implemented what you requested. See latest commit 743bf22a70 .

I've addressed all other comments except this one:

https://github.com/dfinity/ic/pull/2115/#discussion_r1824806762

image

Not sure what that means.

DFINITYManu avatar Nov 05 '24 13:11 DFINITYManu

Not sure what that means.

That was just a recommendation to call the argument disable_setup_checks (as in disable is more commonly used than defeat).

frankdavid avatar Nov 05 '24 14:11 frankdavid