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

activity view: Request events ordered by event ID #3470

Closed
wants to merge 1 commit into from

Conversation

kalikiana
Copy link
Member

  • Give timeago a proper class name
  • Specify ordering in the audit log ajax call
  • Tests check that the ordering is stable

See: https://progress.opensuse.org/issues/58304

@okurz
Copy link
Member

okurz commented Oct 16, 2020

test failures:

[15:32:09] t/ui/16-activity-view.t ................ 1/? 
    #   Failed test 'first job'
    #   at t/ui/16-activity-view.t line 83.
    #          got: 'about an hour ago'
    #     expected: 'about 3 hours ago'

    #   Failed test 'last job'
    #   at t/ui/16-activity-view.t line 85.
    #          got: 'about 10 hours ago'
    #     expected: 'about 12 hours ago'
    # Looks like you failed 2 tests of 7.

#   Failed test 'Current jobs'
#   at t/ui/16-activity-view.t line 86.
# Looks like you failed 1 test of 3.

after you fixed this I would appreciate if you can run more tests, e.g. 100 local test runs to confirm that the test s stable.

@okurz
Copy link
Member

okurz commented Oct 16, 2020

And here as well: Please start the git commit message subject line with a capital letter after the optional component marker.

@okurz
Copy link
Member

okurz commented Oct 16, 2020

I heard from you that you can't reproduce the problem locally. I ran the test module 20 times locally and it never failed.

I think you need to write in t/ui/16-activity-view.t:73

-    }) for keys %fake_events;
+    }) for sort keys %fake_events;

@kalikiana
Copy link
Member Author

I heard from you that you can't reproduce the problem locally. I ran the test module 20 times locally and it never failed.

I think you need to write in t/ui/16-activity-view.t:73

-    }) for keys %fake_events;
+    }) for sort keys %fake_events;

Indeed. And by adding t_created to the event I even tricked myself. The code never actually sorts by time in the first place. So I dropped that and added the sort which should make CI stable.

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.

ugh, please change the commit message to use a capital letter after the optional component marker. Rest is fine. See your PR conditionally approved depending on this tiny change only :)

@kalikiana kalikiana changed the title activity view: request events ordered by event ID activity view: Request events ordered by event ID Oct 16, 2020
@kalikiana kalikiana requested a review from okurz October 16, 2020 15:47
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.

code looks fine but something still seems to be overlooked:

[15:56:58] t/ui/16-activity-view.t ................ 1/? 
    #   Failed test 'first job'
    #   at t/ui/16-activity-view.t line 86.
    #          got: 'about 10 hours ago'
    #     expected: 'about 12 hours ago'
    # Looks like you failed 1 test of 7.

#   Failed test 'Current jobs'
#   at t/ui/16-activity-view.t line 89.
# Looks like you failed 1 test of 3.

@kalikiana kalikiana requested a review from kraih October 20, 2020 07:17
@kalikiana
Copy link
Member Author

Unfortunately tests still don't pass on CircleCI... I changed the unit test to sort the hash keys of the fake job events, and also made sure the intentional "duplicate" event (since in real if you get multiple events for each job) but there is one remaining difference. So I'm still pondering what else could be "undefined"...

@kraih Perhaps you have some ideas on what other "undefined" factors to look for? Or maybe there is a better way to write this test to avoid it

@kraih
Copy link
Member

kraih commented Oct 20, 2020

@kalikiana Could it be that the JSON data in the frontend is simply not sorted and in random order?

@kalikiana
Copy link
Member Author

@kalikiana Could it be that the JSON data in the frontend is simply not sorted and in random order?

Your question made me double-check the JSON once more. And I found out that our DataTables implementation hard-codes the columns which means the answer is No, but by the wrong column 🤦 Fingers crossed this is indeed the last piece I was missing!

@okurz
Copy link
Member

okurz commented Oct 20, 2020

uncross your fingers and please check again :) -> https://app.circleci.com/pipelines/github/os-autoinst/openQA/4566/workflows/ebd6688d-5993-486c-8464-407fa0405415/jobs/43615

    not ok 5 - first job

- Give timeago a proper class name
- Specify ordering in the audit log ajax call
- Tests check that the ordering is stable

See: https://progress.opensuse.org/issues/58304
@okurz
Copy link
Member

okurz commented Nov 3, 2020

@kraih @Martchus maybe you can help to bring this forward? Like take a look at the CI failure, debug in circleCI with ssh, create your own replacement PR, further suggestions to try out, etc.

@kraih
Copy link
Member

kraih commented Nov 3, 2020

I don't work often on the frontend, but i'll take a look. Should be interesting.

@coolo
Copy link
Contributor

coolo commented Nov 3, 2020

What he meant to say:

@kraih
Copy link
Member

kraih commented Nov 3, 2020

Looked at the results in an SSH session and luckily it fails consistently. The results are actually in the correct order, but there seems to be a timezone issue now. Where local results would show 2 hours ago/11 hours ago, it shows an hour ago/10 hours ago on Circle CI.

@kraih
Copy link
Member

kraih commented Nov 3, 2020

If you set TZ=UTC locally it fails the same way as on Circle CI.

@kraih
Copy link
Member

kraih commented Nov 3, 2020

This is one possible solution.

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.

None yet

5 participants