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
contrib: add validation tests using test-agent #2047
contrib: add validation tests using test-agent #2047
Changes from 4 commits
6e77417
cbf126a
cec1413
09f5ea4
d8a16b8
f8c2a1b
c8a61ed
8340e03
adc6bc4
529b2d3
e5f2bac
da09d7e
6bdbc06
14785dc
1a15e04
58ee0cb
95a2e10
a654bbb
a316d59
0c124bc
b417ae3
b498bd9
0a27e33
45d3122
f9be4aa
46723e3
318cf3b
e5706ea
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.
Instead of these super long paths and instead of creating a new
contrib/internal/validationtest/integrations
directory, how about putting the test utils of each contrib in an internal dir under the corresponding and existing contrib package (i.econtrib/<path>/<to>/<dir>/internal
) ?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.
Doing this results in a
use of internal package ... not allowed to compile
error when trying to import the tests within the validationtest file.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.
How about putting the test utils in
contrib/<path>/<to>/<dir>/internal/test
instead ofcontrib/<path>/<to>/<dir>/internal
?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.
Bumping this thread as I believe we can avoid re-creating individual and long contrib folders.
e.g
contrib/internal/validationtest/contrib/miekg/dns/integration.go
should be moved tocontrib/miekg/dns/internal/test/integration.go
. Thecontrib/miekg/dns
dir already exists.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.
The problem is we cannot import an internal sub-package from here since the Go compiler doesn't allow so:
So in this case we have:
contrib/internal/validationtest
And the proposed internal subpackages would be:
contrib/miekg/dns/internal/test
So since the only common root from both directories is
contrib/
, this wouldn't be possible and any shared internal code needs to be placed undercontrib/internal
(sadly for us 😢 ).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.
An extra alternative would be to have these sub-packages not being internal, like
contrib/miekg/dns/test
- not sure how we feel about this approach...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.
Thanks for the explanation @rarguelloF !
I like the idea! We can do it this way for now and move files to separate packages per integration in the future if necessary.
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.
miekg_dns.go, net_http.go, etc can live under
contrib/internal/validationtest
or evencontrib/internal/validationtest/contribs
, right?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.
Since we redeclare the
Integration
struct for each contrib test file, they need to have separate packages and therefore live in separate folders, otherwise I was getting errors trying to put everything in the same directory. I changed the structure to be:contrib/internal/validationtest/miekg/dns.go
,contrib/internal/validationtest/bradfitz/memcache.go
... etcThere 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.
@wconti27 if we follow the big package approach, each struct needs to be named differently as they share the same namespace. e.g.
NetHTTP
, etc.