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.

Sign up for GitHub

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

Jump to bottom

Fix file tests for cobalt. #2252

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Fix file tests for cobalt. #2252

merged 1 commit into from
Feb 28, 2024

Conversation

aee-google
Copy link
Contributor

b/316404107

@aee-google aee-google requested a review from sherryzy January 19, 2024 21:32
sherryzy
sherryzy reviewed Jan 22, 2024
View reviewed changes
Copy link
Contributor

@sherryzy sherryzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding to the change in this PR, it's a little bit similar to my other change, @andrewsavage1 made a comment #2241 (comment). Not sure if we want to implement some of these Starboard functions instead of disabling them.

base/files/file_enumerator_unittest.cc Outdated Show resolved Hide resolved
@aee-google
Copy link
Contributor Author

Adding functionality that does not currently exist seems mostly out of scope. Cobalt doesn't depend on this behavior. If we wanted to introduce this functionality for cobalt or to reduce the starboard/cobalt specific code, that can be done outside of the update (either before or after the update).

@aee-google aee-google requested a review from sherryzy January 22, 2024 22:08
@andrewsavage1
Copy link
Contributor

Adding functionality that does not currently exist seems mostly out of scope. Cobalt doesn't depend on this behavior. If we wanted to introduce this functionality for cobalt or to reduce the starboard/cobalt specific code, that can be done outside of the update (either before or after the update).

I can't speak more generally, but base::Move is used in a number of places in upstream code, i.e. //net and //components. Implementing it here will reduce the number of changes we need to make to actually get everything to build

@aee-google
Copy link
Contributor Author

I'll look into implementing base::Move in main.

@aee-google aee-google requested a review from a team as a code owner February 26, 2024 22:06
@aee-google
Copy link
Contributor Author

@andrewsavage1 This is updated and ready for a look.

andrewsavage1
andrewsavage1 reviewed Feb 27, 2024
View reviewed changes
base/BUILD.gn Outdated Show resolved Hide resolved
base/BUILD.gn Show resolved Hide resolved
base/BUILD.gn Show resolved Hide resolved
base/files/file_unittest.cc Outdated Show resolved Hide resolved
base/files/file_util.h Outdated Show resolved Hide resolved
base/test/BUILD.gn Outdated Show resolved Hide resolved
base/files/file.h Show resolved Hide resolved
@aee-google aee-google merged commit 7c115b0 into youtube:feature/all-upstream-update Feb 28, 2024
124 of 260 checks passed
@aee-google aee-google deleted the fix-file_unittest branch February 28, 2024 23:13
sherryzy added a commit to sherryzy/cobalt that referenced this pull request Mar 6, 2024
@sherryzy
Fix UploadFileElementReaderTest
3b8d5d4 
In this PR: youtube#2252, we add
delete_on_close_ to fix file_unittest.cc. However, I don't think
that's the right approach and it breaks UploadFileElementReaderTest.
Currently, those FileTest is disabled in trunk. Revert that change
and leave it as it is for now.

b/327654843
@sherryzy sherryzy mentioned this pull request Mar 6, 2024
Merged
sherryzy added a commit to sherryzy/cobalt that referenced this pull request Mar 7, 2024
@sherryzy
Fix UploadFileElementReaderTest
cd34236 
In this PR: youtube#2252, we add
delete_on_close_ to fix file_unittest.cc. However, I don't think
that's the right approach and it breaks UploadFileElementReaderTest.
Currently, those FileTest is disabled in trunk. Revert that change
and leave it as it is for now.

b/327654843
sherryzy added a commit to sherryzy/cobalt that referenced this pull request Mar 7, 2024
@sherryzy
Fix UploadFileElementReaderTest
8597f24 
In this PR: youtube#2252, we add
delete_on_close_ to fix file_unittest.cc. However, I don't think
that's the right approach and it breaks UploadFileElementReaderTest.
Currently, those FileTest is disabled in trunk. Revert that change
and leave it as it is for now.

b/327654843
sherryzy added a commit that referenced this pull request Mar 7, 2024
@sherryzy
Fix UploadFileElementReaderTest (#2552)
4aa01a8 
In this PR: #2252, we add
delete_on_close_ to fix file_unittest.cc. However, I don't think that's
the right approach and it breaks UploadFileElementReaderTest. Currently,
those FileTest is disabled in trunk. Revert that change and leave it as
it is for now.

b/327654843
andrewsavage1 pushed a commit to andrewsavage1/cobalt that referenced this pull request Mar 7, 2024
@sherryzy @andrewsavage1
Fix UploadFileElementReaderTest
090461b 
In this PR: youtube#2252, we add
delete_on_close_ to fix file_unittest.cc. However, I don't think
that's the right approach and it breaks UploadFileElementReaderTest.
Currently, those FileTest is disabled in trunk. Revert that change
and leave it as it is for now.

b/327654843
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsavage1 andrewsavage1 andrewsavage1 approved these changes

@sherryzy sherryzy Awaiting requested review from sherryzy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aee-google @andrewsavage1 @sherryzy