-
Notifications
You must be signed in to change notification settings - Fork 143
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
starboard/nplb: Fold nplb_evergreen_compat_tests into nplb proper #4946
starboard/nplb: Fold nplb_evergreen_compat_tests into nplb proper #4946
Conversation
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 think this is reasonable and LGTM but I'll leave it to Holden who knows more about these tests
These tests are tricky and I think it unfortunately would be more trouble than it's worth to move them into They were never designed to run in CI and there are several reasons why they can't currently be expected to pass in CI across our reference devices: https://b.corp.google.com/issues/292138589#comment2. We could add filters for the problematic tests, but we'd probably end up filtering over half of them. We may end up simply removing these tests soon. They were originally intended as a quick way for partners to assess progress in their Evergreen integrations, and their test coverage is very minimal compared to the Evergreen YTS tests. I need to revisit b/292138589 and make a proposal, but I think what we'll end up doing is retiring these tests and adding any extra coverage they provide to YTS. Also, if we we do want to land this PR then we'll need to update a few partner docs (e.g., https://github.com/youtube/cobalt/blob/main/starboard/doc/evergreen/cobalt_evergreen_overview.md, https://developers.google.com/youtube/cobalt/docs/development/setup-android). |
I like this part. If these tests are nplb, then they should run there. Otherwise it's hard to justify their existence; the point of nplb is to be a single integration test. |
07e7af5
to
181395e
Compare
@hlwarriner and I talked about this. Next PR proposal is:
@y4vor FYI |
Just want to clarify something about
What's kind of missing, and is highlighted by issue 2 from https://b.corp.google.com/issues/292138589#comment2, is something to capture whether an |
To further the point of this CL, and perhaps erroneously, this is my understanding:
Ofc |
No, I don't think that's quite right. A couple examples from 25.lts.1+ of evergreen-compatible platform and evergreen platform pairs, from our reference ports:
|
The meaning of these flags is the following: |
c19fee2
to
724a860
Compare
Gotcha thanks for the explanation. I've changed the code to be more specific: SKIP those 3 test cases on platforms where we don't have the paths. Currently this is only Linux, since there it doesn't make sense to have to create e.g. |
724a860
to
5343c27
Compare
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.
Left one comment requesting a small doc change but otherwise lgtm.
Let's just wait for approval from @y4vor , too.
…on-EVERGREEN (full) platforms
…een_compat_tests that don't apply.
- CrashpadConfigTest.VerifyUploadCert was IIUC broken due to an extra ...app/cobalt/content... path manually added to the cert location. Looking at the GN ssl:copy_ssl_certificates target, it seems to agree with GetCACertificatesPath(). The test logic is fixed and extended. The class is unnecessary, so it's removed. - FontsTest.* needed deps on fonts:copy_font_data GN targets (both data_deps and deps). This worked, but I had to comment out the `install_content = true` statements in that BUILD.gn (unclear whether this would break other things). Said GN target uses exec_script and needs an exception in //.gn.
…ing copy_ssl_certificates_evergreen target. Removed unnecessary Gtest classes
0da60e0
to
960a0d4
Compare
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.
@y4vor PTAL
@andrewsavage1 I had to do something fishy in starboard/content/fonts/BUILD.gn -- would you mind taking a look..? Thanks!
…after chatting with Andrew
…content/ssl/BUILD.gn
1e5d5f6
to
a07f133
Compare
This is a follow-up to #4831, rolling
nplb_evergreen_compat_tests
intonplb
proper.Run via
./out/linux-x64x11_devel/nplb --gtest_filter=*CrashpadConfigTest*:*SabiTest*:*ExecutableMemoryTest*:*MaxFileNameTest*:*StorageTest*:*LoaderAppMetricsTest:*FontsTest*
--Two sets of test failed on my Linux and needed attention:
CrashpadConfigTest.VerifyUploadCert
was IIUC broken due to anextra
...app/cobalt/content...
path manually added to the certlocation. Looking at the GN
ssl:copy_ssl_certificates
target, itseems to agree with
GetCACertificatesPath()
in that the extra pathis not needed. Discussing this with @y4vor, we decided to add a new
helper target
copy_ssl_certificates_evergreen
to put the certsin the right spot.
FontsTest.*
neededdeps
onfonts:copy_font_data
GN targets(both
data_deps
anddeps
). This worked, but I had to commentout the
install_content = true
statements in that BUILD.gn (it'sunclear whether this would break other things). Said GN target
uses
exec_script
and needs an exception in //.gn, this CL addsthat too.
Asides from it, some unnecessary test classes are removed, and some
preprocessor logic simplified.
Bug: b/384819454