Issue 172 dev: Add file upload test data #205
Conversation
|
Thanks for the PR @mattburnett-repo ! It's adding almost 60 audio files to the repository. Is that intentional? |
|
yes, the extra files are there because they are processed by the file upload endpoint, and therefore playable in Beam, et al. |
| .set('Authorization', `Bearer ${testAccessToken}`) | ||
| .attach('files', `${audioSourceFilePath}${fileName}`) | ||
|
|
||
| expect(response.status).to.eql(200) |
There was a problem hiding this comment.
How is it checking that the file actually exists?
Is this cleaning up uploads afterwards? If not, will this just create new files every single time tests are run?
There was a problem hiding this comment.
Oh I just read the comment at the top. This should probably not be named a test.js file right if it is a seed script? Otherwise the tests will automatically run it.
Are there tests that get run?
There was a problem hiding this comment.
FileUpload.test.js is not a legitimate test file. It's only used to generate all of the test data. It shouldn't be run as a test file, as mentioned in the code comments and the 'HowTheFileUploadDataWasMade' file.
There is no cleanup, since we need the audio files to provide sound when the user clicks on a track.
My understanding of the issue was that we needed to seed the test/dev Docker container with everything needed to allow a user to click on a track and hear the associated audio. Basically this issue was about just adding more test data, similar to how we already seed the database.
There was a problem hiding this comment.
Gotcha, that makes sense.
In that case though, does it make sense for it to be in the test directory? It's not a test. Couldn't we just run it as a js script? Does it need to run inside of mocha to use supertest for it, is that why it needs to be in the test directory? My worry is that right now it might accidentally get run as part of the test suite, just because of its location and format. Then people would get into a situation where they're indefinitely running the tests.
I'm seeing 30 tests, which makes sense for the amount of tracks on the albums. Could you point to me where there's another 30 or so files needed?
There was a problem hiding this comment.
Ii am not sure about what to do with the test file at this point. I'd actually like to get rid of it, now that we have the needed data. This is for the reason/s you already mention.
However, it would be needed in the future if, for some reason, the test data had to be re-created. Also, I prefer to keep everything that contributes to the evolution of the codebase. It's a tool, albeit one that wouldn't / shouldn't be run often, if at all.
It doesn't have to be in the test directory. Alternatively, it could remain where it is, and be excluded by adding a few lines to (I think) the mocha config file. For now, everything in the file is set to 'skip', so nothing in the file actually executes.
I don't understand what you mean by "another 30 or so files". We already have the needed files; I don't understand why we would need another 30.
There was a problem hiding this comment.
Ok, here's what I see as needing to change for this PR:
- Remove FileUpload.test.js and its folder
- Remove the 30-something [trim] files from test/media/audio
- Remove HowTheFileUploadDataWasMade.md file, and any references to it
Let me know if this is correct, or if there's more to be done. Once you confirm, I can push the changes.
There was a problem hiding this comment.
Also, there's a broken test somewhere in the test suite, as the result of this PR. Although it's related, it looks like a separate matter. Once we sort out this PR and merge, I will fix the test/s.
There was a problem hiding this comment.
I think you can keep the trim files, those get used when a user isn't logged in, other than that, looks good!
Would it be a lot of effort to fix the test?
There was a problem hiding this comment.
The text talks about using FileUpload.test.js. Since we decided to delete this file, it made little sense to me to keep anything talking about the test data creation.
There was a problem hiding this comment.
Re-added the trim files. There is now a separate PR for this issue.
|
closed cause there's another pr for this now |
This PR addresses the lack of test data for the API file upload functionality, described in issue #172.
Test data has been added to the Sequelize database seeder scripts, and test audio files that can be played by the Resonate Beam application are added.
A brief description of how this data was created is provided in the HowTheFileUploadTestDataWasMade.md file.