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

Fixes #26968 - Switch to minitest-ci #7997

Closed
wants to merge 2 commits into from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Sep 15, 2020

ci_reporter_minitest is unmaintained and since minitest 5.1 it no longer shows the class names. minitest-ci does.

Submitting as a draft to see what the result on Jenkins is. I chose minitest-ci over minitest-reporters since it appears to be lighter, especially when it comes to Rails. However, perhaps developers prefer the other.

@theforeman-bot
Copy link
Member

Issues: #26968

@ekohl
Copy link
Member Author

ekohl commented Sep 15, 2020

@ezr-ondrej could you have a look at the test results and how it's reported? I find it odd that the number of tests also differs between Ruby versions. There also appear to be differences between how the declaration style (let, describe, it) vs method style (def test_*).

@ezr-ondrej ezr-ondrej self-assigned this Sep 23, 2020
@ezr-ondrej
Copy link
Member

[test foreman]

1 similar comment
@ezr-ondrej
Copy link
Member

[test foreman]

@ezr-ondrej
Copy link
Member

[test katello]

ezr-ondrej
ezr-ondrej previously approved these changes May 12, 2021
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I'm ok getting this in 👍
I do not see much of a difference here, so let's get to supported reporter.

@ares
Copy link
Member

ares commented Nov 10, 2021

Hello @ekohl, given this is a draft for a very long time we'd like to know if you have a plan to move this out of the draft. It seems it's ready to merge, once you switch it.

If we don't get a reply in 2 weeks, we'll close the PR but it can always be reopened.

@ekohl
Copy link
Member Author

ekohl commented Nov 10, 2021

I think I completely missed @ezr-ondrej's approval. I'll rebase and move it out of draft. I'd prefer to merge it post branching.

@ekohl ekohl force-pushed the 26968-minitest-ci branch 2 times, most recently from bd3f60b to 3563aaa Compare November 10, 2021 10:39
@ekohl ekohl marked this pull request as ready for review November 10, 2021 10:39
@ezr-ondrej
Copy link
Member

[test katello]

3 similar comments
@ezr-ondrej
Copy link
Member

[test katello]

@ezr-ondrej
Copy link
Member

[test katello]

@ezr-ondrej
Copy link
Member

[test katello]

@ekohl
Copy link
Member Author

ekohl commented Nov 16, 2021

I wonder why this happens:

2021-11-15T15:33:03 [W|app|] Failed to register foreman-tasks plugin (FATAL:  database "test_develop_pr_katello-0-dev" does not exist

@ezr-ondrej
Copy link
Member

I wonder why this happens:

It's right after database:drop task, but I do not know why the next task is running in production

@ezr-ondrej
Copy link
Member

[test katello]

@ezr-ondrej
Copy link
Member

[test katello] I wonder why it fails here so often, but it passes on other PRs 🤔

@ezr-ondrej
Copy link
Member

[test katello]
do it!

@ezr-ondrej
Copy link
Member

[test katello]
and do not get stuck :/

@ezr-ondrej
Copy link
Member

[test katello]
I do not like how flaky this is

@ezr-ondrej
Copy link
Member

[test katello]

1 similar comment
@ezr-ondrej
Copy link
Member

[test katello]

@ezr-ondrej
Copy link
Member

[test katello]
Does it behave now?

@ezr-ondrej
Copy link
Member

[test katello]

@ezr-ondrej
Copy link
Member

[test katello]
I'm beggining to suspect this actually breaks the katello build, but no idea how it could be true

@ezr-ondrej
Copy link
Member

[test katello]

end
minitest_plugins = [:pre_ci, 'ci:setup:minitest']
minitest_plugins = [:pre_ci]
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that katello enhances the ci:setup:minitest to do some weird custom stuff?
Find ci:setup:minitest didn't come up with anything, but there might be some codependency on task that Katello uses?

@ezr-ondrej
Copy link
Member

As @evgeni pointed out, I can't read logs and this is actually failing to report katello results with the new reporter.

@@ -11,10 +10,10 @@ begin

namespace :setup do
task :pre_ci do
ENV["CI_REPORTS"] = 'jenkins/reports/unit/'
gem 'ci_reporter'
require 'minitest/ci'
Copy link
Member

@evgeni evgeni Dec 1, 2021

Choose a reason for hiding this comment

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

Suggested change
require 'minitest/ci'
ENV['USE_MEAN_TIME_REPORTER'] = '0'
require 'minitest/ci'

This might be what makes Katello confuse the shit out of Minitest::Ci:

https://github.com/Katello/katello/blob/30e6fe7d91d6296bb02830e4afaa3f66bee38795/test/katello_test_helper.rb#L12-L15

lib/tasks/jenkins.rake Outdated Show resolved Hide resolved
@evgeni
Copy link
Member

evgeni commented Dec 1, 2021

[test katello]

@ezr-ondrej
Copy link
Member

I believe this still needs a rebase to latest develop

ekohl and others added 2 commits December 1, 2021 13:26
ci_reporter_minitest is unmaintained and since minitest 5.1 it no longer
shows the class names. minitest-ci does.
@evgeni
Copy link
Member

evgeni commented Dec 1, 2021

rebased, let's see

@evgeni
Copy link
Member

evgeni commented Dec 1, 2021

[test katello]

@evgeni
Copy link
Member

evgeni commented Dec 1, 2021

in this PR, foreman works, but katello fails. and in #8960 foreman fails but katello works.

MADNESS

@ezr-ondrej
Copy link
Member

Given we are green on #8960 I'm in favor of going that route.

@evgeni
Copy link
Member

evgeni commented Dec 2, 2021

yeah, #8960 is merged, so let's close here.

Thanks @ekohl for starting this!

@evgeni evgeni closed this Dec 2, 2021
@ekohl ekohl deleted the 26968-minitest-ci branch December 2, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants