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 bug with SQL query for analysis view #483

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

ChrisPaulBennett
Copy link
Contributor

@ChrisPaulBennett ChrisPaulBennett commented Aug 14, 2023

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3fdfafa) 76.95% compared to head (94ae9ce) 76.95%.

Additional details and impacted files
@@           Coverage Diff           @@
##            1.3.x     #483   +/-   ##
=======================================
  Coverage   76.95%   76.95%           
=======================================
  Files          12       12           
  Lines        1406     1406           
  Branches      233      233           
=======================================
  Hits         1082     1082           
  Misses        277      277           
  Partials       47       47           
Files Changed Coverage Δ
cylc/uiserver/schema.py 70.80% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisPaulBennett
Copy link
Contributor Author

This bug was noticed in a code review (cylc/cylc-ui#1355)

@MetRonnie MetRonnie changed the base branch from master to 1.3.x August 14, 2023 09:53
@MetRonnie MetRonnie added the bug Something isn't working label Aug 14, 2023
@MetRonnie MetRonnie added this to the 1.3.1 milestone Aug 14, 2023
@oliver-sanders
Copy link
Member

Hi there, thanks for this fix.

You've merged master into this branch, but the PR is being raised against 1.3.x, as a result there are a bunch of unrelated commits from the 1.4.0 branch (master) showing on this PR.

The easiest way to proceed is probably to cherry-pick those commits onto 1.3.x e.g:

$ git fetch upstream
$ git checkout -b fix-sql-bug upstream/1.3.x
$ git cherry-pick 3e80eedb25a311c3ecc5e9fae5fe450d56fea930
$ git cherry-pick 6b3e99fcaa71f3cf945ef3fd9ff4c9ca529b1932
$ git cherry-pick 9b8b94d93a2f92c9d448629e70875ffb313b93c2

Check the diff is as expected:

$ git diff upstream/1.3.x

You can avoid re-opening this PR by pushing to origin/master e.g:

# override your remote master with this branch
$ git push -f origin HEAD:master

@MetRonnie
Copy link
Member

MetRonnie commented Aug 14, 2023

Thanks, can you just extract commit 3e80eed? May have to create a feature branch and open a new PR

$ git checkout -b sql-fix upstream/1.3.x # (if the remote is called upstream)
$ git cherry-pick 3e80eedb25a311c3ecc5e9fae5fe450d56fea930

@MetRonnie MetRonnie changed the title Fix bug with SQL Query Fix bug with SQL query for analysis view Aug 16, 2023
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

LGTM, though I'm not sure how to test/reproduce the bug, may need @oliver-sanders to review too

@oliver-sanders
Copy link
Member

I'm not sure how to test/reproduce the bug, may need @oliver-sanders to review too

I think this should do it, try viewing a workflow with:

  • A single failed task script = false.
    • The analysis view should not list any times for this task (run status != 0).
  • Then broadcast script = sleep 60 and re-trigger the task.
    • The task should now show in the analysis view.
    • The run time should be ~60s not ~30s (because the failed task has been excluded).

@MetRonnie
Copy link
Member

Ok, tested, reproduced bug on 1.3.x but all good on this branch

@MetRonnie MetRonnie merged commit b1f3e1a into cylc:1.3.x Aug 21, 2023
4 checks passed
MetRonnie added a commit to MetRonnie/cylc-uiserver that referenced this pull request Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants