fabric8io / mockwebserver

An extension of okhttp's mockwebserver, that provides a DSL and is easier to use
Apache License 2.0
113 stars 38 forks source link

Major project refactor and clean-up #66

Closed manusa closed 2 years ago

manusa commented 2 years ago

/cc @shawkins Most of the things needed downstream have been exposed. I'm not sure if I may be leaving something out.

shawkins commented 2 years ago

@manusa I didn't have a particular need beyond exposing the namespace attribute. The only other thing I can think of that the mock can't handle, which can be handled separately of course, is that repeated calls to the same websocket url don't work - that is once you upgrade to a websocket and it gets closed the underlying executor gets closed and can't be used for a subsequent call to the same url.

manusa commented 2 years ago

@manusa I didn't have a particular need beyond exposing the namespace attribute. The only other thing I can think of that the mock can't handle, which can be handled separately of course, is that repeated calls to the same websocket url don't work - that is once you upgrade to a websocket and it gets closed the underlying executor gets closed and can't be used for a subsequent call to the same url.

Do you have an example for this?

I'm assuming you might be talking about this part: https://github.com/fabric8io/mockwebserver/blob/6e9eb1bebb1c20594aca6b571f4fa7e42f9a2d6c/src/main/java/io/fabric8/mockwebserver/internal/WebSocketSession.java#L160-L172

manusa commented 2 years ago

The idea is to release after merging this PR and be able to finally perform some of the awaited changes and improvements downstream.

shawkins commented 2 years ago

Here's an example with a modified WatchTest:

  @Test
  void testWithTimeoutSecondsShouldAddQueryParam() throws InterruptedException {
    // Given
    server.expect()
      .withPath("/api/v1/namespaces/test/pods?timeoutSeconds=30&watch=true")
      .andUpgradeToWebSocket().open()
      .waitFor(EVENT_WAIT_PERIOD_MS).andEmit(new WatchEvent(pod1, "DELETED"))
      .waitFor(EVENT_WAIT_PERIOD_MS).andEmit(outdatedEvent()).done().always();

    // When
    final CountDownLatch eventReceivedLatch = new CountDownLatch(2);
    Watch watch = client.pods().watch(new ListOptionsBuilder().withTimeoutSeconds(30L).build(), new Watcher<Pod>() {
      @Override
      public void eventReceived(Action action, Pod resource) { eventReceivedLatch.countDown(); }

      @Override
      public void onClose(WatcherException cause) { }
    });

    Watch watch1 = client.pods().watch(new ListOptionsBuilder().withTimeoutSeconds(30L).build(), new Watcher<Pod>() {
      @Override
      public void eventReceived(Action action, Pod resource) { eventReceivedLatch.countDown(); }

      @Override
      public void onClose(WatcherException cause) { }
    });

    // Then
    assertTrue(eventReceivedLatch.await(3, TimeUnit.SECONDS));
    watch.close();
    watch1.close();
  }

I'm assuming you might be talking about this part

Yes, I didn't track it down fully, but it looks like that executor is shutdown even when "always" is specified.

manusa commented 2 years ago

Hi @shawkins The last commit should take care of your scenario. Could you please provide your review?