Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Factory plugin implementation #566
Factory plugin implementation #566
Changes from 198 commits
99f45d1
075a4cf
ecbf407
8fb11b6
032dbba
953e953
6185edb
cded6ad
fbfe523
0d70179
cdb7efe
f15bb47
59359a2
f63df91
406bd6b
f0b697b
8425e48
fb4f372
c6eae6c
96ae38b
b17240f
5c5a146
29591ff
7d928a9
ff82d95
98381a4
83ac556
38c5084
fa4e285
8551c76
437de5e
6cef87e
c84ba4d
77d33f9
73aa686
ef77d65
b9182a0
e38dd80
21734e4
7cda9d2
f827aca
d194736
7820cf9
bee7805
3db68ce
c0ec4dd
a1dd3f2
0b25261
c3fa23c
547977e
c300203
8f4d4d9
5ae2d51
fe74b6d
6d87aea
ca7a0c2
967e3f8
14b7d79
c4985bb
922639a
81dfece
bf08661
34900c5
768fa81
e93bb58
f47cbd4
7c5c6b2
1b02cec
4d75602
c74f535
9949fb8
fcf10d3
fd9744d
68c1037
883ee30
8c34b25
6ce7e74
92927e5
a62f2d5
16f89c6
f4257e3
d075588
eb10a3b
cdbd6bf
17acc5f
55a0e62
fa23aad
b8e2447
248f439
91703b5
bbc0c2a
517c83d
c1d0428
bc86c09
33e3564
338a874
402ada2
017481f
3ed43fc
247b71b
718e4cd
50c0c07
171fb2e
5df83ed
0af3ffd
ff90e59
e57c05f
55244b0
3d0b1a2
c710d37
fbb2fdc
ae0f4a8
e092de0
1b87b71
6a72987
a080eec
19b7aa0
dae3098
a4b6fb1
55f685e
59c5b94
2ea0c5e
d40a073
7146079
7ab1132
72c23d0
2b1170c
9dc3c4b
b555a71
3924eee
75eca07
9c93ff9
0040b33
e2af886
edae9be
949d731
59d490f
078329a
63e5d2b
530cc9c
22cc3f8
32489d0
324bb23
5d6bb26
32ae13d
12708ac
eff2f4e
b890df4
a907913
49a47a4
b711028
8a2d792
22b93aa
7b9c175
bab49fc
7184695
fc7eda6
49e6eec
43de3d8
fef08dd
264355c
fa4d4c9
546449a
1a28a30
fa2adc5
01959ea
6696b5e
6168223
3043405
309f75b
4a63c18
55a7460
a1646e6
eababba
f935dbc
1aa000a
e471e2b
edfdbf3
fdb84d2
592345a
e98211f
436f952
fee4328
c37cde2
8e56850
ac18ebb
66cd5ed
fc254f0
9a409dd
314f924
587cf3c
8401dd5
f25fd1e
77782cd
95070b1
5034b19
5c4a717
8937c6a
835a42e
177b16c
6d4323f
f73c20c
d04aa5e
3be39ae
932de71
e9572a8
b6b5b64
dc5d9c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe annotate with a
XXX_GCOVR_EXCL_XXX
. We don't care not every branch gets hit in tests, putting that in documents an explicit choice that we're ok with that. If it would ever start working, it would also help with correcting the reported coverage % (it seems to be ignored today).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I'm not sure what you mean by this. Are you referring to adding a: NOT_REACHED_GCOVR_EXCL_LINE; statement inside of the Release_assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on slack, we could probably just append
// NOT_REACHED_GCOVR_EXCL_LINE
to the line itself, or useThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be private instead or do we have a good reason to make it protected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this can be private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Can we replace some of these auto statements with the actual type? Only needed in places where the type isn't obvious from the expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, I wonder if it would make sense to parameterize this test over plugin setup. So we iterate the same test over all known plugins. That way, future plugins could add themselves to this hypothetical parameterized list, and have basic integration test coverage through that. (This would imply that all plugin types would be set up with a configuration that yields the same traffic pattern). Wouldn't object to leaving this as is, but if you see sufficient value it might be worth doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.