Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Tests and linting CI #42

Closed
wants to merge 13 commits into from
Closed

Tests and linting CI #42

wants to merge 13 commits into from

Conversation

Henriquelay
Copy link
Collaborator

Obviously depends on #27, tests action currently fail on my fork, but it is similar enough to the linting action to guarantee it will work, once testing is in place.

I did on a separate fork because it is a lot of actions CI trial-and-error, and did not want to commit-spam. It was the easiest proven working method to try stuff with GitHub CI. So, as to avoid commit-spam, we should squash this before merging.

@Henriquelay Henriquelay requested a review from AtilioA May 24, 2022 18:43
@Henriquelay
Copy link
Collaborator Author

That's funny, it even tested itself before the merge. Wasn't expecting that. I guess I could have done it through a PR.

This exemplifies my claims. You can see the test action fails because there is no such command, and lint fails because of an actual linting error.

I can rebase #27 here once done to avoid merge a failing CI to the trunk.

@AtilioA
Copy link
Owner

AtilioA commented May 24, 2022

That's funny, it even tested itself before the merge. Wasn't expecting that. I guess I could have done it through a PR.

Yes, this also happens for #33. How these PRs differ apart from linting?
I'm not sure if linting should be added to CI as it will originate plenty of trivial failures.

Other than that, seems legit.

@Henriquelay
Copy link
Collaborator Author

How these PRs differ apart from linting?

For starters, this one works. #33 fails to install dependencies. This one uses yarn instead of npm, the same we use to develop the application, uses cache action, and only builds v16. Mostly, this one does not implement codecov. We could change that for the interest of the project. Yeah, this should be a append to #33, but I haven't had a closer look on it until now.

I'm not sure if linting should be added to CI as it will originate plenty of trivial failures.

Linting errors shouldn't be ignored. Even when they are accepted by the remote (pre-commit hook shouldn't allow), I think a CI fail tag to become clear that this specific version is failing linting could be very handy. Also, if one is having trouble with linting errors, or a way of using a tool doesn't meet out styling principles, ignore directives are a thing.

@AtilioA
Copy link
Owner

AtilioA commented May 24, 2022

I don't think there are benefits for running linters on CI, in contrast to running tests, for example. All this is gonna do is activate bad neurons with all the ❌es

@Henriquelay
Copy link
Collaborator Author

All neurons are bad neurons. But sure, I can just remove the lint job

@AtilioA
Copy link
Owner

AtilioA commented May 24, 2022

Can you merge these yamls into #33? I have not taken care of errors there because we didn't even have tests, but we'll want code coverage.

@Henriquelay
Copy link
Collaborator Author

Just taking the Codecov job should work, but we need to know the parameter to set to point to server and front separately

Henriquelay added a commit that referenced this pull request May 24, 2022
Codecov remains completely untested, since we need tests first
@Henriquelay Henriquelay mentioned this pull request May 24, 2022
AtilioA pushed a commit that referenced this pull request Aug 10, 2022
Codecov remains completely untested, since we need tests first
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants