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.

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

[Epic] Anticheat runtimes (EAC and BattlEye) #1560

Merged
merged 27 commits into from
Jul 17, 2022
Merged

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Jul 9, 2022

It is now possible to manage and enable anticheat runtimes in the launcher
Current issues:

  • BattlEye runtime doesn't work, the Wine-GE patch for it seems to not apply. Not sure why, needs more testing
    Nevermind, the runtime works, the only game I was testing with (The Cycle: Frontier) did not work correctly

Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@CommandMC CommandMC added the pr:wip WIP, don't merge. label Jul 9, 2022
@CommandMC CommandMC self-assigned this Jul 9, 2022
Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

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

Beside the error evaluation, this is a clean code.
But the error handling is important, so need to reject the pr.
How you handle the errors is your choice. You can do it in the backend or in the frontend.

Btw you could also write backend tests. Downloading/unzipping etc. You can have a look at heroic-wine-downloader tests, how i simulate a url request, download etc.

electron/wine/runtimes/ipc_handler.ts Show resolved Hide resolved
electron/wine/runtimes/runtimes.ts Show resolved Hide resolved
electron/wine/runtimes/runtimes.ts Outdated Show resolved Hide resolved
electron/wine/runtimes/runtimes.ts Outdated Show resolved Hide resolved
electron/wine/runtimes/runtimes.ts Outdated Show resolved Hide resolved
electron/wine/runtimes/util.ts Show resolved Hide resolved
electron/wine/runtimes/util.ts Outdated Show resolved Hide resolved
electron/wine/runtimes/util.ts Show resolved Hide resolved
src/screens/Settings/components/WineExtensions/index.tsx Outdated Show resolved Hide resolved
src/screens/Settings/components/WineExtensions/index.tsx Outdated Show resolved Hide resolved
@CommandMC
Copy link
Collaborator Author

CommandMC commented Jul 10, 2022

Wasn't done with this yet, otherwise I would've marked it as ready for review

I understand your concerns and will implement error handling in the new functions. As for tests, other than examples, do you have a piece of documentation or something I can look at, to make sure I'm doing it right?

@Nocccer
Copy link
Collaborator

Nocccer commented Jul 10, 2022

Wasn't done with this yet, otherwise I would've marked it as ready for review

I understand your concerns and will implement error handling in the new functions. As for tests, other than examples, do you have a piece of documentation or something I can look at, to make sure I'm doing it right?

Ah sorry didn't saw the wip. Ok i will resolve all comments for now.
Jest documentation: https://jestjs.io/docs/getting-started
Everything is setup already. You just need to create a new *.test.ts and write test's there.
If you need help i can try to write the first test for you as an example.

@CommandMC
Copy link
Collaborator Author

Alright, error handling is now in place. Let me know what you think, I'll take a look at tests now then

CommandMC and others added 8 commits July 14, 2022 15:40
The stacktrace was, for some reason, showing the wrong lines, which
was confusing to work with. I'd say its better to just hide it for now
Unrelated, but this removes `yarn.lock` changes from the diff generated
by git, which could sometimes have huge changes when people mix-and-match
`npm` and `yarn`
@CommandMC CommandMC added pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:wip WIP, don't merge. labels Jul 17, 2022
@CommandMC
Copy link
Collaborator Author

There are still a lot of tests missing, although the functionality itself might be nice to have for a beta release

@flavioislima flavioislima merged commit d15812b into beta Jul 17, 2022
@flavioislima flavioislima deleted the feat/ac-runtimes branch July 17, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants