Feature new utility: RequestInterrupter (for tests only)

[fredg1]

Tests-wise, there's currently no way to debug outgoing GenericRequests.

Any method that ends with / has logic that centers around a call to RequestThread.postRequest is therefore pretty much un-testable.
So, I was wondering if there would be any demand for something which would try to help with exactly that.

Basically, I'm thinking of adding conditionals at various points, like at the start of GenericRequest.run(), at the start of GenericRequest.execute() and before entering GenericRequest.externalExecute().
If a RequestInterrupter was enabled, these conditionals could allow things like stopping the request in order to simply examine it (enabling tests like "this method should make a request to fight.php"), or to bypass GenericRequest.externalExecute(), while injecting it with a responseText manually.

Of course, even though this is only meant for tests, the nature of GenericRequest means that it would require applying modifications to src/ ...

Ideas/thoughts?

 

[fronobulax]

My recollection of Ye Olden Days is that the team members who really knew Mockito were able to do the same thing and train others, by example.

So... Automated tests are good but automated tests that leave the source untouched are better.

The "adding conditionals at various points" reminds me of a discussion of Feature Flags which in my addled memory were conditionals that enabled or disabled user visible functionality. If we elect to go with this I'd like to be sure that a user could not (deliberately or not) flip a conditional and then "break" their KoLmafia session.

 

[heeheehee]

The specific use cases you outline should be doable by using HttpUtilities.setOpen with something that returns a mock ConnectionFactory. (In the case where you're constructing and specifically testing a Request, you can directly use a spy for the object as in RequestTestBase.)

I'm willing to entertain this possibility, but I'd prefer to keep the existing test surface area low unless we need it. Test-only functionality can be useful but risks ossification.

 

[fredg1]

In the case where you're constructing and specifically testing a Request, you can directly use a spy for the object as in RequestTestBase.

I did stumble upon this when looking up where the conditionals could go.
Wondered if it meant that what I wanted to add was already available, but quickly understood that it required you to be the one constructing the Request, which here, was not always the case...

The specific use cases you outline should be doable by using HttpUtilities.setOpen with something that returns a mock ConnectionFactory.

Care to point me to documentation/help/details on the subject?

 

[fronobulax]

My recollection of Ye Olden Days is that the team members who really knew Mockito were able to do the same thing and train others, by example.

Care to point me to documentation/help/details on the subject?

+1 since I'm not sure I trust my memory and there are pieces out there.

 

[heeheehee]

setOpen allows you to override how we construct HttpUrlConnections from URLs, as used internally by GenericRequest and a few other places in our codebase.

If I were to use it for this purpose, I might do something like this:

class FakeConnection extends HttpUrlConnection {
  FakeConnection(int responseCode, String responseText) { ... }
  ...
}

var factory = mock(ConnectionFactory.class);
HttpUtilities.setOpen(factory);

var connection = new FakeConnection(200, "hello world");
when(factory.openConnection(argThat(url -> url.getPath().equals("mall.php")))).thenReturn(connection);

// do stuff
...

There's definitely room to make this more user-friendly (e.g. by providing a standard implementation for FakeConnection, or creating reusable matchers for URL).

 

[fredg1]

@heeheehee looking into HttpUtilities & ConnectionFactory; why exactly do you need to make a *mock* of ConnectionFactory?

Looking at your example, can't you just replace your logic with

class FakeConnection extends HttpUrlConnection {
  FakeConnection(int responseCode, String responseText) { ... }
  ...
}

HttpUrlConnection connection = new FakeConnection(200, "hello world");

HttpUtilities.setOpen(url -> {
  if (url.getPath().equals("mall.php")) {
    return connection;
  }
  return null;
});

// do stuff
...

?

 

[heeheehee]

You could. But you mentioned wanting to verify that this was actually called in the first place, right?

(enabling tests like "this method should make a request to fight.php")

Yes, you could set a boolean in the if statement, or a counter, or whatever else. Or you could just verify() that the method was called on that mock.

Registering actions is easier to read and less error-prone than passing a giant lambda, especially if you need to handle multiple interactions.

Technically you don't need to use Mockito to orchestrate any interactions. But it provides a consistent interface so you don't need to write custom logic for every single test.

 

[fredg1]

@heeheehee tried something in https://github.com/kolmafia/kolmafia/pull/175

1- how does it look?
2- If it seems good, any idea of a test I could use/make to test the concept?