Improve JUnit support

These are small improvements to merged JUnit support for Merge Requests:

• We should not send Poll-Interval when data is returned. It makes FE to periodically re-request it. Polling should happen while we are parsing,
• We might want to figure out how we support pipeline when we retry jobs. It seems that test results will be cached for them, but not refreshed,
• We might want to reduce caching time for 10 minutes, as I'm not sure what impact it is gonna have on Redis,
• Fix runner features support for shells: gitlab-runner!989 (merged),
• Documentation: https://gitlab.com/gitlab-org/gitlab-ce/issues/49867,
• [-] Feature specs FE+BE: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20972, => Unified with https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21039
• Development fixtures: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20973

changed the description

We might want to figure out how we support pipeline when we retry jobs. It seems that test results will be cached for them, but not refreshed

• maybe we should use updated_at of both pipelines as additional arguments for with_reactive_cache? this increases memory consumption :(
• maybe your idea for including updated_at in response and invalidating the cache is the good one. I like your idea better, as it keeps memory consumption.

@shinya @filipa

http://recordit.co/6SgSENB9xV

The behavior after you push a change to MR:

1. The first failure (at begin) gonna be solved by fixing Poll-Interval behavior: frontend will stop pulling test results,
2. The second failure (at end), but we still are able to expand tests seems to be frontend bug.

@filipa it seems that when we show failure on frontend, but next request is succeeded the failure message persist.

It seems that Poll-Interval and behavior of frontend can be fixed with:

  def test_reports
    result = @merge_request.compare_test_reports

    case result[:status]
    when :parsing
      Gitlab::PollingInterval.set_header(response, interval: 3_000)

      render json: '', status: :no_content
    when :parsed
      render json: result[:data].to_json, status: :ok
    when :error
      render json: { status_reason: result[:status_reason] }, status: :bad_$
    else
      render json: { status_reason: 'Unknown error' }, status: :internal_se$
    end
  end

@shinya I propose that we reduce interval to 3s. Reports usually take 1s to parse. Making it 10s we make the worse UX.

@filipa Can you verify if the it seems that when we show failure on frontend, but next request is succeeded the failure message persist is correct?

assigned to @filipa

@ayufan

We might want to figure out how we support pipeline when we retry jobs. It seems that test results will be cached for them, but not refreshed,

I wonder if we can invalidate the cache in ExpirePipelineCacheWorker. The intention of the class is "If pipeline updated, then the cached results should be renewed, too". I feel this concept totally falls into our subject.

diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb
index 992fc63c451..774d1507713 100644
--- a/app/workers/expire_pipeline_cache_worker.rb
+++ b/app/workers/expire_pipeline_cache_worker.rb
@@ -22,6 +22,15 @@ class ExpirePipelineCacheWorker
     end
 
     Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline)
+
+    if pipeline.has_test_reports?
+      pipeline.all_merge_requests.each do |merge_request|
+        merge_request.expire_reactive_cache!(
+          :compare_test_results,
+          merge_request.base_pipeline&.iid,
+          merge_request.actual_head_pipeline.iid)
+      end
+    end
   end
 
   private

Or, put the eveevent in before_transition any => [:success, :failed, :canceled] do |pipeline| (i.e. when pipeline finished)

• We might want to reduce caching time for 10 minutes, as I'm not sure what impact it is gonna have on Redis,

I'm ok with the value. If we can smartly invalidate cached resutls of compared test reports, we might be able to keep the 1 hour.

@ayufan I also found a bug that Ci::Pipeline#test_reports targets all builds (latest + retried), so I'm going to include the fix as well. This is necessary for invalidating cache properly.

Here is the outcome of the above fix. It properly invalidates the cached test reports and re-calculate the new results.

To automatically render the test reports section, we need to restart polling. (In the above gif, I reloaded forcibly)

mentioned in merge request !21039 (merged)

I created https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21039. Let's fix both FE+BE together in the branch (Sorry it's named improve-junit-support-be but we should fix in the same place as the size of changes will be smaller than we had)

I'll also include feature spec into the branch. So all we need to merge in %11.2 is the branch.

changed the description

I’m not sure about cache invalidation as it should support base and head pipelines together, and if we can do it without cache invalidation it is preferred as we forget about the need to evaluate another edge cases.

added Deliverable label

1. The second failure (at end), but we still are able to expand tests seems to be frontend bug. @filipa it seems that when we show failure on frontend, but next request is succeeded the failure message persist.

We show a failure when the status code is different from 2XX

@ayufan @dosuken123 which code is being sent?

@filipa I just checked the behavior. It seems after the BE fix, the

The second failure (at end), but we still are able to expand tests seems to be frontend bug.

automatically resolved.

So I don't think we need any actions from FE here

@filipa

But it seems that if you constantly pool and show the error, the MR widget will not recover from error state after subsequent run. MR widget will re-poll when it receives test_path: nil and next test_path: something.

@ayufan yes, that's what supposed to happen. It stops polling after an error. that is how the polling works.

There is no FE work on the issue description, I'm un assigning myself

unassigned @filipa

@filipa...

But it restarts polling when it receives test_path: again.

It doesn't restart polling - that only happens after a hard refresh

The only way it can restart is: status code is 2xx and you change tabs and come back again to this tab

@filipa It fails in the following case:

1. test_reports returns error,
2. test_reports_path: nil,
3. test_reports_path: value,
4. test_reports returns success,
5. failure of loading persist

@ayufan

In order to try to fix whatever is causing number 5 I need instructions on how to

1. have the reports locally -they are not on gdk
2. steps to reproduce that

@ayufan please add instructions of a real life example of going from

test_reports returns error,
test_reports_path: nil,

and from

1. test_reports_path: nil,
1. test_reports_path: value,

without refreshing the page

@filipa

1. You have pipeline with invalid XML report,
2. You open MR page,
3. You push a new Pipeline with updated JUnit,
4. You wait for this failing.

@ayufan Can you please me teach me how to

1. set up test reports on gdk
2. have an invalid xml report

?

@filipa cc @shinya

@filipa I've added two fixtures to reproduce @ayufan's concern and my concern.

Please follow this instruction https://gitlab.com/snippets/1731728#note_92840235. The fixtures are ready in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21039.

FYI, here are the results when I tested. Both cases was able to be reproduced.

Example 4 (https://gitlab.com/gitlab-org/gitlab-ce/issues/49966#note_92479615)

Example 5 (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21039#note_92620370)

/cc @ayufan

Hey @dosuken123 and @erushton, how's this work coming? Still on track for 11.2 with the code freeze today?

@jlenny Yes, we're going to merge some improvements (Bug fix + Performance improvements) in %11.2.

mentioned in merge request !21067 (merged)

closed via merge request !21039 (merged)

mentioned in commit 0e90f27f

mentioned in issue #46520 (moved)