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

Added a new 'Run all' button to interactive toolbar on the web interface #981

Merged
merged 14 commits into from
Aug 24, 2023

Conversation

rnemes
Copy link
Contributor

@rnemes rnemes commented Aug 10, 2023

Bug / Requirement Description

Clearly and concisely describe the problem.

Solution description

Describe your code changes in detail for reviewers.

Checklist:

  • Test
  • Example (both test_plan.py and .rst)
  • Documentation (API)
  • News fragment present for release notes
  • MS info leakage check
  • For new driver: driver index page
  • For new assertion: ui/pdf/std renderers, documentation
  • For new cmdline arg: documentation

@rnemes rnemes changed the title Added a new 'Run all' button to Toolbar on the web interface Added a new 'Run all' button to interactive toolbar on the web interface Aug 10, 2023
@rnemes rnemes marked this pull request as ready for review August 10, 2023 13:08
@rnemes rnemes requested a review from Pyifan as a code owner August 10, 2023 13:08
@rnemes rnemes requested review from kn-ms and M6AI August 10, 2023 15:00
@@ -72,6 +74,7 @@ class InteractiveReportComponent extends BaseReport {
navWidth: `${INTERACTIVE_COL_WIDTH}em`,
resetting: false,
reloading: false,
running: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

So I looked at this new status and wonder two things:

  • Can we do without it?
  • If not, then we need to block certain actions when it is True. Right now I think we can trigger it twice without problem, send a reload during it, reset, etc... See for instance, how the reloadCode in InteractiveReport is handling it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we do without it?

We only need that for logging the state of the progress. However, we could use another variable eg.: runtime_status which could replace resetting, reloading, aborting as well, but in that case need to refactor the related functionalities.

If not, then we need to block certain actions when it is True. Right now I think we can trigger it twice without problem, send a reload during it, reset, etc... See for instance, how the reloadCode in InteractiveReport is handling it.

That makes sense. I will update the code.

this.putUpdatedReportEntry(updatedReportEntry);
this.setState({ running: true });
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Once the fixes for the active filter issues are in, we probably need to see if it is working with a filter expression. If not, we will need to extend support, I do not think that piece of the API is handling the filtering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't handle filtering, it starts everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned offline, this will be needed.

Copy link
Contributor

@M6AI M6AI left a comment

Choose a reason for hiding this comment

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

Minor update but more of an ask for some deep-dive, let us follow up on it offline.

@rnemes rnemes requested a review from M6AI August 23, 2023 18:12
for test_uid in self.all_tests():
self.run_test(test_uid)
if shallow_report:
self.logger.debug("Interactive mode: Run filtered tests")
Copy link
Contributor

Choose a reason for hiding this comment

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

Loglines are a welcome addition, but I do not see it for the others. Also I wonder if it is INFO level already or perhaps USER_INFO.

Copy link
Contributor

@M6AI M6AI left a comment

Choose a reason for hiding this comment

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

Let us align offline too, but overall LGTM.

M6AI
M6AI previously approved these changes Aug 24, 2023
Copy link
Contributor

@M6AI M6AI left a comment

Choose a reason for hiding this comment

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

Tested offline too.

</NavItem>
);
} else {
if (props.filter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (pops.filter) {
  title="Run filtered tests"
} else {
  title = "Run all tests"
}
return (
  <NavItem key="runall-button">
          <div className={css(styles.buttonsBar)}>
            <FontAwesomeIcon
              key="toolbar-runall"
              className={css(styles.toolbarButton)}
              icon={faPlay}
              title={title}
              onClick={props.runAllCbk}
            />
          </div>
        </NavItem>
)

@rnemes rnemes requested a review from yuxuan-ms August 24, 2023 12:03
@rnemes rnemes merged commit 7fa3d3f into morganstanley:main Aug 24, 2023
@rnemes rnemes deleted the RunAllInteractive branch August 24, 2023 14:17
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