Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

B2MD verifier #939

Merged
merged 2 commits into from May 10, 2019

Conversation

Projects
None yet
5 participants
@weissi
Copy link
Member

commented Apr 1, 2019

Motivation:

When writing B2MDs, there are a couple of scenarios that always need to
be tested: firehose feeding, drip feeding, many messages, ...

It's tedious writing those tests over and over again for every B2MD.

Modifications:

Add a simple B2MD verifier that users can use in unit tests.

Result:

Hopefully fewer bugs in B2MDs.

@weissi weissi requested a review from normanmaurer Apr 1, 2019

@ktoso

ktoso approved these changes Apr 1, 2019

Copy link
Member

left a comment

Looks good/useful enough to include in core :)

Show resolved Hide resolved Sources/NIO/Codec.swift Outdated

@weissi weissi added this to the 2.1.0 milestone Apr 1, 2019

@Lukasa
Copy link
Contributor

left a comment

This generally looks really good, but I have a few notes about a bit of the implementation.

Show resolved Hide resolved Sources/NIO/Codec.swift Outdated
Show resolved Hide resolved Sources/NIO/Codec.swift Outdated
Show resolved Hide resolved Sources/NIO/Codec.swift Outdated
expected: expectedOutput)
}
}
let actualExtraOutput = try channel.readInbound(as: Decoder.InboundOut.self)

This comment has been minimized.

Copy link
@Lukasa

Lukasa Apr 2, 2019

Contributor

Should this be while let to obtain the full overproduction?

typealias Out = Decoder.InboundOut

func verifySimple(channel: EmbeddedChannel) throws {
for (input, expectedOutputs) in inputOutputPairs.shuffled() {

This comment has been minimized.

Copy link
@ianpartridge

ianpartridge Apr 2, 2019

Contributor

Wouldn't this make tests non-deterministic?

This comment has been minimized.

Copy link
@weissi

weissi Apr 2, 2019

Author Member

@ianpartridge Yes, it does. The expectation is that B2MDs shouldn't mind that and if they do that's a bug. I could add (in the error) the specific input that was sent maybe?

It's a similar issue like when relying on the order of the key/value pairs in a dictionary I guess.

This comment has been minimized.

Copy link
@ianpartridge

ianpartridge Apr 2, 2019

Contributor

Yes I'm just thinking about how users could debug their B2MD when faced with an order-dependent test failure.

This comment has been minimized.

Copy link
@weissi

weissi Apr 2, 2019

Author Member

@ianpartridge well, it's one B2MD that they'd need to look at and it needs to be state that persists across two messages (of which there should be very little). However, the whole thing could be solved by just giving the user the data in a failed attempt. Then they can make that particular input a regression test by itself.

This comment has been minimized.

Copy link
@ianpartridge

ianpartridge Apr 2, 2019

Contributor

Yep, sounds good to me.

This comment has been minimized.

Copy link
@Lukasa

Lukasa Apr 2, 2019

Contributor

Can we instead accept a hash to affect the shuffling, and then print that hash in the event of test failure?

This comment has been minimized.

Copy link
@weissi

weissi Apr 2, 2019

Author Member

@Lukasa Sure, that works too.

This comment has been minimized.

Copy link
@weissi

weissi May 8, 2019

Author Member

@Lukasa / @ianpartridge I added the delivery of the inputs, does that work?

@weissi weissi force-pushed the weissi:jw-b2md-verifier branch 2 times, most recently from 18152a4 to 0c4f8ec May 8, 2019

@weissi weissi requested review from Lukasa and normanmaurer May 8, 2019

@Lukasa
Copy link
Contributor

left a comment

Really good! Just a few nitty things here.

@Lukasa Lukasa modified the milestones: 2.1.0, 2.2.0 May 9, 2019

Create NIOTestUtils & add B2MD verifier
Motivation:

When writing B2MDs, there are a couple of scenarios that always need to
be tested: firehose feeding, drip feeding, many messages, ...

It's tedious writing those tests over and over again for every B2MD.

Modifications:

- Add a simple B2MD verifier that users can use in unit tests.
- Add a new, public `NIOTestUtils` module which contains utilities
  mostly useful for testing. Crucially however, it does not depend on
  `XCTest` so it can be used it all targets.

Result:

Hopefully fewer bugs in B2MDs.

@weissi weissi force-pushed the weissi:jw-b2md-verifier branch from 0c4f8ec to a61ed59 May 10, 2019

@Lukasa

Lukasa approved these changes May 10, 2019

@weissi

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

@swift-nio-bot test this please

@weissi

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

16:45:27 removal of container a462f06554d90c3e6f6b04d1ddc6a72ee249d60df04e493d84ced4bffed4a67c is already in progress
16:45:27 Build step 'Execute shell' marked build as failure

addressed comments, want to write the nio-extras code.

@weissi weissi merged commit 5513bb2 into apple:master May 10, 2019

2 checks passed

pull request validation (5.0) Build finished.
Details
pull request validation (5.1) Build finished.
Details

@weissi weissi deleted the weissi:jw-b2md-verifier branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.