Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6175

HistoryServerTest.testFullArchiveLifecycle fails

    Details

      Description

      https://s3.amazonaws.com/archive.travis-ci.org/jobs/213933823/log.txt

      estFullArchiveLifecycle(org.apache.flink.runtime.webmonitor.history.HistoryServerTest)  Time elapsed: 2.162 sec  <<< FAILURE!
      java.lang.AssertionError: /joboverview.json did not contain valid json
      	at org.junit.Assert.fail(Assert.java:88)
      	at org.junit.Assert.assertTrue(Assert.java:41)
      	at org.junit.Assert.assertNotNull(Assert.java:712)
      	at org.apache.flink.runtime.webmonitor.history.HistoryServerTest.testFullArchiveLifecycle(HistoryServerTest.java:98)
      

      Happened on a branch with unrelated changes Chesnay Schepler.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user zentol opened a pull request:

          https://github.com/apache/flink/pull/3655

          FLINK-6175 Harden HistoryServerTest#testFullArchiveLifecycle

          This PR hardens the `testFullArchiveLifecycle` test by introducing an optional `CountdownLatch` argument to the HistoryServer constructor This latch is updated after each finished archive poll, regardless of whether it failed or not.

          This simplifies the test and removes edge cases where the requested file already exists but the contents weren't written yet.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/zentol/flink 6175_hs_test

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3655.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3655


          commit 3e491a532330476ecf05f2eb319a95212569b52d
          Author: zentol <chesnay@apache.org>
          Date: 2017-03-23T12:58:03Z

          FLINK-6175 Harden HistoryServerTest#testFullArchiveLifecycle


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user zentol opened a pull request: https://github.com/apache/flink/pull/3655 FLINK-6175 Harden HistoryServerTest#testFullArchiveLifecycle This PR hardens the `testFullArchiveLifecycle` test by introducing an optional `CountdownLatch` argument to the HistoryServer constructor This latch is updated after each finished archive poll, regardless of whether it failed or not. This simplifies the test and removes edge cases where the requested file already exists but the contents weren't written yet. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zentol/flink 6175_hs_test Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3655.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3655 commit 3e491a532330476ecf05f2eb319a95212569b52d Author: zentol <chesnay@apache.org> Date: 2017-03-23T12:58:03Z FLINK-6175 Harden HistoryServerTest#testFullArchiveLifecycle
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user haohui commented on the issue:

          https://github.com/apache/flink/pull/3655

          Looks good to me overall. Instead of changing in the normal code path, maybe it is a little bit less intrusive to use Mockito to change the field? For example:

          ```
          JobArchiveFetcherTask task = Whitebox.getInternal(archiveFetcher, "fetcherTask");
          JobArchiveFetcherTask spiedTask = Mockito.spy(task);
          Whitebox.setInternal(archiveFetcher, "fetcherTask", spiedTask);
          doAnswer(answer -> latch.countDown()).doCallRealMethod().when(spiedTask).run();
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user haohui commented on the issue: https://github.com/apache/flink/pull/3655 Looks good to me overall. Instead of changing in the normal code path, maybe it is a little bit less intrusive to use Mockito to change the field? For example: ``` JobArchiveFetcherTask task = Whitebox.getInternal(archiveFetcher, "fetcherTask"); JobArchiveFetcherTask spiedTask = Mockito.spy(task); Whitebox.setInternal(archiveFetcher, "fetcherTask", spiedTask); doAnswer(answer -> latch.countDown()).doCallRealMethod().when(spiedTask).run(); ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3655

          It would be less intrusive, that's true. I do prefer staying away from mockito magic as far as possible though Mockito often results in failing tests even though no public contract was modified/violated: in those 4 lines alone you rely on 3 internal details to not change for the test to succeed (name of the Field, class of the Field, way of execution (run()).

          This now means that when the implementation changes so must the test, which kind of defeats their purpose of comparing a new implementation to a known use-case; as well as introducing a risk for additional bugs to sneak in.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3655 It would be less intrusive, that's true. I do prefer staying away from mockito magic as far as possible though Mockito often results in failing tests even though no public contract was modified/violated: in those 4 lines alone you rely on 3 internal details to not change for the test to succeed (name of the Field, class of the Field, way of execution (run()). This now means that when the implementation changes so must the test, which kind of defeats their purpose of comparing a new implementation to a known use-case; as well as introducing a risk for additional bugs to sneak in.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3655

          While at hardening: The test seems to work with a fix port. That is bound to cause failures due to conflicts (for example 8082 is common to be used if you have a local HA Flink for testing).

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3655 While at hardening: The test seems to work with a fix port. That is bound to cause failures due to conflicts (for example 8082 is common to be used if you have a local HA Flink for testing).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3655

          @StephanEwen Good idea, updated the PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3655 @StephanEwen Good idea, updated the PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3655

          merging.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3655 merging.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3655

          hmm...this didn't work as well as I'd hoped; its still failing on travis. Will investigate a bit more.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3655 hmm...this didn't work as well as I'd hoped; its still failing on travis. Will investigate a bit more.
          Show
          till.rohrmann Till Rohrmann added a comment - Another failure: https://s3.amazonaws.com/archive.travis-ci.org/jobs/232433964/log.txt
          Hide
          Zentol Chesnay Schepler added a comment -

          1.3: 4e7598d9c21821db8f03a84d0c2e64959b651d38
          1.4: 5ba5cd5ac9ab00da18b723177fb8c977c56e009c

          Show
          Zentol Chesnay Schepler added a comment - 1.3: 4e7598d9c21821db8f03a84d0c2e64959b651d38 1.4: 5ba5cd5ac9ab00da18b723177fb8c977c56e009c
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3655

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3655

            People

            • Assignee:
              Zentol Chesnay Schepler
              Reporter:
              uce Ufuk Celebi
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development