tremor-cli/test: Refactor large parts of the test code. #1305
Conversation
MordecaiMalignatus
requested review from
anupdhml,
darach,
Licenser and
mfelsche
as code owners
MordecaiMalignatus
force-pushed
the
refactor
branch
from
b1558f2 to
085afb9
Compare
|
I'm confused by the CI error, the only appearance of that string is in before.rs and it seems to have something to do with how long the tests take? Given that it's in the same area it's something I maybe broke on accident, but I'm not sure how. |
MordecaiMalignatus
force-pushed
the
refactor
branch
2 times, most recently
from
bf092d0 to
9bb8ccd
Compare
Pull Request Test Coverage Report for Build 1393410269
💛 - Coveralls |
This gets rid of one of the long functions mentioned in tremor-rs#20, makes the code more readable, and (in the future) easier to test properly. Signed-off-by: Mordecai Malignatus <mordecai@malignat.us>
MordecaiMalignatus
force-pushed
the
refactor
branch
from
9bb8ccd to
959afe4
Compare
|
Found the problem, I broke the tag selection. I'd revert back to a draft PR but that doesn't seem to be a thing. I'll dig into it this evening. I'll try to write automated tests for it and refactor to make it testable, because this is not going to be maintainable without. |
|
All good, converting to a draft is well hidden :) it's op the top right under the approvers: I've turned it back to a draft for you |
|
Thanks!
… On 28. Oct 2021, at 10:24, Heinz N. Gies ***@***.***> wrote:
All good, converting to a draft is well hidden :) it's op the top right under the approvers:
<https://user-images.githubusercontent.com/119093/139216837-8e843074-b12e-489e-ac50-704f1e5e5649.png>
I've turned it back to a draft for you
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#1305 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAF62QO7FNIW5HCFYWOXDVDUJEJFNANCNFSM5G4A3SIQ>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Heya any updats on this? |
|
I've been working on finishing it when I could, but currently short on time. I'll try to curtail it and ship it, I have some ideas for the larger renovation/change of this bit. Sorry for the delay :) |
|
All good :), no hurry just wanted to know if you're still on it or have given up ;) |
|
I currently don't have the free time or the energy to finish this, sorry. I'll leave it as it is and, if I'm lucky, have the time to finish up the rest of this over the winter. Apologies for leaving it hanging. |
|
All good no worries:) we appreciate it! |
Pull request
Description
This is a refactoring of large parts of
test.rs, in order to make the code more readable and to get rid of one of the very long functions mentioned in #20. It's essentially a spillover from my other patch at #1283.Tests are still rather hard to write for this (since I didn't touch the functionality), but it's starting to be easier to test and it might end up with being able to test parts of the testing module. I've done my best to
There is no external impact to this change except for 1-2 added messages when something is skipped.
Related
Checklist
Performance
Performance impact should not occur, and if anything be very mildly positive from removing a couple of duplicate checks :)