Improve test-support

[gossi]

When working on #620 my new discovered some shortcomings of the current test-support. This is a spin-off of the mentioned PR to discuss ideas to improve it :)

Utilize Test-Support ;)

Currently, the test-support for mirage is part of the public API for production code. It should be moved test-support to not pollute prod builds with that chunk of code.

Specifically: move addon/mirage to somewhere into addon-test-support

Multiple Adapters

At the moment, ember-file-upload ships with a handler for mirage. Would be nice to have support for polly.js, too.

Even better, I think equally imporant are the test-support utilities, such as extractFormData() and extractFileMetadata(). They are very much suitable for integration tests, ie.

// understand this as pseudo-code, I'm not writing this from memory (API and names are likely to be different)
test('it uploads (with polly)', async function(assert) {
  this.server.polly.post('/photos/new', (req, res) => {
    extractFileMetadata(req.responseBody);
  });
});

As I created those tests myself without/prior to knowing those utils exists, they are likely to work with either polly as well as mirage :)

Imperatively Control the Upload Progress

Writing UIs around upload is definitely part of userland code. Giving developers a good way to test their UIs is key. To test what happens in each file.state? Using await selectFiles() would skip the uploading state and would require devs to either waitFor() of some dom node to appear or set some timeout on the request itself. Both are not reliable and have high potential to cause flaky tests, read more in this discussion.

An idea would be to have an imperative API to programmatically advance the state and be in control. Basically, a deferred promise - this is shim code:

test('it uploads (with polly)', async function(assert) {
  const uploadHandler = new PollyUploadHandler();
  this.server.polly.post('/photos/new', async (req, res) => {
    return uploadHandler.handle((req, res) => {
      // business logic code
    });
  });

  await render(hbs`...`);

  const upload = selectFilesForHandler('input[type="file"]', new File(...));

  uploadHandler.startUpload(upload); // moves `state` into `uploading`

  assert.dom('...').hasText('uploading...'); // asserts UI state for uploading

  uploadHandler.success((req, res) => { 
    // possible business logic code
  });

  // --- OR ---

  uploadHandler.fail(); // example for not making use of business logic
  
  assert.dom('...').hasText('upload failed'); // assert UI for `error` state
});

This is a rough idea but I myself have too many questions how to shape the API around that, when and where to allow business logic, etc. My hope is to get the discussion going.

[jelhan]

Utilize Test-Support ;)

Currently, the test-support for mirage is part of the public API for production code. It should be moved test-support to not pollute prod builds with that chunk of code.

Specifically: move addon/mirage to somewhere into addon-test-support

I also was about to propose this several times. But the mirage route handler are not only needed in test environment. It is needed if Mirage is enabled - regardless of the environment. If using default configuration, this is the case for development environment. In some edge cases Mirage may be even used in production.

Multiple Adapters

At the moment, ember-file-upload ships with a handler for mirage. Would be nice to have support for polly.js, too.

As an alternative we could move adapter for mirage into a separate addon, which could be installed if needed. This would also reduce some of the complexity regarding Mirage and Embroider caused by this route handlers.

Even better, I think equally imporant are the test-support utilities, such as extractFormData() and extractFileMetadata(). They are very much suitable for integration tests, ie.

Not sure if I would make them part of the public API to be honest. They are adding a lot of complexity while only adding value for some users. I wouldn't be that much concerned about making them public API if moving them out into standalone package before.

Imperatively Control the Upload Progress

As far as I know Mirage does not have a good solution for deferring and manual resolving regular HTTP requests. Does polly.js has one? As per the other points I fear the complexity added to this library, which already has limited maintaining capacities. I guess being able to iterate on such ideas quicker would be another good argument to move it out into a separate package.

[gilest]

I like the idea of deprecating the mirage handler and providing it through an optional separate addon. If we want we could still manage it from this repo once we convert to monorepo.

[gossi]

Thanks @jelhan for explaining the reasons around mirage. As I want to start a monorepo as first PR to #642 - this would fit this situation.

I've also realized, this is probably too much business logic for providing imperative control for tests. Instead of providing functionality, as there are libraries available to do that already - this should be handled by docs, showing examples how to do that 👍

[RobbieTheWagner]

@gossi did you make any progress on tests? Progress calculation is broken after 8.2.0, so we clearly don't have tests that check a real upload flow to catch this.

[gossi]

No, not invested further than the investigation. But it needs an API that you can programmatically change the state, or make use of rerender, which would make it feel home in ember.