electron / libchromiumcontent

Shared library build of Chromium’s Content module
MIT License
486 stars 183 forks source link

Work to fix libcc unit tests #483

Closed nitsakh closed 6 years ago

nitsakh commented 6 years ago

This PR makes changes to address libcc unit test issues. (#360 and #443)

nitsakh commented 6 years ago

I've just fixed the content_browsertests build and the existing test right now. But we should probably add tests to test our patch code.

alexeykuzmin commented 6 years ago

Are these changes related to any of our patches? If yes, they should be added to that patch.

alexeykuzmin commented 6 years ago

Can you please rebase your branch onto the latest master?

470 was merged several hours ago and it's added a new config for patches, so when you add a new patch it also has to be added to that config to be actually applied.

I'll update the documentation soon.

deepak1556 commented 6 years ago

Are these changes related to any of our patches? If yes, they should be added to that patch.

Second that, will be easy to reason the changes.

alexeykuzmin commented 6 years ago

Also, can you please add your Slack email to your Github account? GitHub doesn't know that "nitsakh@slack-corp.com" is you at the moment )

nitsakh commented 6 years ago

I should've committed with the other email address. Will amend the commit to change the email address. Changes are related to another patch, but I was thinking that keeping test changes separate could be a good idea.

alexeykuzmin commented 6 years ago

Changes are related to another patch, but I was thinking that keeping test changes separate could be a good idea.

Reasons?

nitsakh commented 6 years ago

Yeah, that was the initial thought, but having them in the same patch is the right thing to. I've updated the patch.

nitsakh commented 6 years ago

Following tests are also failing after running the run_tests script.

Maybe some of these are not needed, but these were all the failures I got after running the run_tests script.
 We should probably remove some targets if we do not need them.

deepak1556 commented 6 years ago

Its weird that extensions_browsertests got built, there shouldn't be a target in our test build config that has it as a dep, it should be disabled if its getting built. I am good with having rest of the components, as we use parts of them in Electron. We can disable the tests that can't be fix at the moment.

alexeykuzmin commented 6 years ago

Conflicts.

alexeykuzmin commented 6 years ago

Let's merge it. CI builds are green, so at least you didn't break anything (yet) =)

nitsakh commented 6 years ago

How did I manage to break all of CI? 🤔

alexeykuzmin commented 6 years ago

@nitsakh

disable-warning-win.patch failed to apply

alexeykuzmin commented 6 years ago

@nitsakh It seems like Windows build are OK. Should we merge the PR?

nitsakh commented 6 years ago

Yes, let's merge the PR. We can then get this in CI. If there are some things, we can work on those after the merge.