Conversation
helenosheaa
commented
Jan 20, 2021
- Removed tests for deprecated plugins - eg kafka_legacy
- Fixed certain integration tests / short unit tests that were failing
- Commented out tests that were consistently failing on "test-all" - left comment above test with reason
- Added command test-integration to makefile and added "Integration" to all integration tests so they can be run separately
- Created documentation of integration tests, status and future integration tests which would be beneficial
- Added to dockerfile to remove warning from mysql
- There are integration tests which could be deleted given they test third party libraries but there aren't many other tests / in some cases none in that case -some examples of this - nats/ mqtt/ nsq
- There are integration tests which could be deleted given they test third party libraries that are currently failing - some examples of this - opencua / openldap
…oved legacy int test, add command to run only integration tests, md doc on integration tests
reimda
left a comment
There was a problem hiding this comment.
Thanks for going through all these tests! I like the system of adding the suffix "Integration" to the tests that run with docker-compose.
I'd prefer not to use comment out the tests that aren't working but that we don't want to fix immediately. Since commented out code doesn't get compiled it's easy for it to break over time. It's also hard to know in the future why sections are commented out.
Instead of commenting out, let's disable them by calling t.Skip first thing. I see that you added comments with the errors each failing test produces. Let's move those comments to be right next to the skip.
I would probably also just disable the tests for deprecated plugins instead of removing them.
|
|
||
| func TestTable(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("Skipping integration test in short mode") |
There was a problem hiding this comment.
I wrote this test and meant for the snmp agent to be set up manually, not with docker-compose like the others. Sorry not to add a comment to make this clear. Since it's not set up to run with docker-compose we shouldn't add the Integration suffix to the func name.
There was a problem hiding this comment.
No problem, I've removed the Integration from the end of the name and added a skip for until it's fixed.
|
Thanks for the review! I've changed it to skip the ones which aren't working with the reason inside the skip. I'll create an issue which lists the broken ones as discussed instead of it being within the docs. I've also just gone for disabling the legacy integration tests as well as suggested. |