test: exploreUtils #13719
test: exploreUtils #13719
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13719 +/- ##
==========================================
+ Coverage 77.39% 78.56% +1.17%
==========================================
Files 928 935 +7
Lines 47016 47375 +359
Branches 5719 5964 +245
==========================================
+ Hits 36386 37222 +836
+ Misses 10487 10010 -477
Partials 143 143
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/src/explore/exploreUtils/getExploreLongUrl.test.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/exploreUtils/getExploreUrl.test.ts
Outdated
Show resolved
Hide resolved
|
@yardz About the comments.. one way to achieve the same objective of communicating the problems that you had, is to mark this PR as a draft and make the comments in the review section. This way we can iterate on each problem and keep the code clean 😉 |
I agree, but I think that in this context it is a little more complicated. If we are going to solve all the problems that I mentioned, I believe that PR will be gigantic. This file has several functions with very dangerous behaviors. I created the tests and skipped them just to warn of these behaviors. The idea here was to approve what can be tested but the tests are there to show some behaviors that can generate a bug. Changing the functions, adding typescript and improvements is not the goal now ... So I thought it best not to discuss this in this PR. |
| * For some reason the spy is not working properly and I also did not find a way to mock only the "postForm" | ||
| * I believe that if this file is divided the problem will be solved. | ||
| */ | ||
| test.skip('Should call postForm correctly', () => { |
There was a problem hiding this comment.
exploreChart is just calling getExploreUrl and postForm. I don't think is necessary to create a test for exploreChart. Just testing getExploreUrl and postForm is enough.
| @@ -236,6 +241,9 @@ export const buildV1ChartDataPayload = ({ | |||
| export const getLegacyEndpointType = ({ resultType, resultFormat }) => | |||
| resultFormat === 'csv' ? resultFormat : resultType; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
One way to test postForm is to mock document.createElement returning an object that contains a monitored submit function. Then we can test if all the required fields (action, method, token, data) are correctly configured when submitting. We can also test that the form is added hidden and removed after submit.
If you think that the PR will be giant I suggest just keeping in this PR the tests that are working and the code without those comments. Then open another PR with the problematic tests in draft mode. |
superset-frontend/src/explore/exploreUtils/shouldUseLegacyApi.test.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/exploreUtils/getExploreUrl.test.ts
Outdated
Show resolved
Hide resolved
* Tests for exploreUtils * removing eslint-disable * applying factory * Removing skip tests and code comments. * Improving test name * remove comments * Improving "shouldUseLegacyApi" tests * fixing typo
* Tests for exploreUtils * removing eslint-disable * applying factory * Removing skip tests and code comments. * Improving test name * remove comments * Improving "shouldUseLegacyApi" tests * fixing typo
mistercrunch
added
🏷️ bot
SUMMARY
Creation of tests for the functions in the file.
Some functions could not be tested correctly, for these cases I created the test that and put the command skip.
Along with that I put a comment explaining why that test is not working.
I put the test of each function in a file to facilitate understanding and prevent the context of the tests from mixing and generating unwanted behaviors.