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

Add initial continuous integration #3604

Merged
merged 2 commits into from
Jul 31, 2020
Merged

Add initial continuous integration #3604

merged 2 commits into from
Jul 31, 2020

Conversation

hoshsadiq
Copy link
Contributor

Currently it will run phpunit, check coding standards and run
phpstan. This can and should be build upon, so that eventually builds
are automated and hopefully reproducible.

Furthermore, I've not been up to date with current community standards, nor
am I aware of any coding standards for Firefly, however, I have added coding
standards check on a whim in the hopes of opening a conversation, so I'm happy
to change to whatever is the best standards these days.

Lastly I added phpstan to improve code quality. There are lot of issues it's
returning (over 8000), though I'm not sure how valid they are. I've yet to
start to fix any issue.

I'm hoping in the future we can add additional automation to the whole
project so that the maintenance overheads of this project is reduced to a
minimum.

Lastly, I'm planning to as much of this as possible but this is going to be a
big task, so I'd very much appreciate your help as you understand the codebase
better. I think fixing the database fixtures should fix many, if not all tests.

Currently it will run phpunit, check coding standards and run
phpstan. This can and should be build upon, so that eventually builds
are automated and hopefully reproducable.
ci.env Show resolved Hide resolved
ci.env Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
@JC5
Copy link
Member

JC5 commented Jul 28, 2020

Thanks, this is very nice! 👍

Some comments and opinions from my side:

  • I'm a stickler for the name, it's "Firefly III", not "Firefly".
  • I'm happy to see what PHPStan and other static analysers come up with (SonarCloud is also installed) but I couldn't really care less about their results. 8000 positive results is a sign of 7000 false positives and duplicates, if PHPStan is anything like any static analyser out there.
  • I have a similar opinion about code standards. My own PHPStorm installation is pretty strict already. I'm not going to fight a tool over where my brackets are. But I don't mind being told about missing docblocks.

As for the whole thing about fixtures. I currently run the tests locally in conjunction with a script that generates a fresh database every time. It's what powers the demo site too; it generates new and up-to-date transactions daily. I used to ship a test database as well but the last time I did CI using Travis CI I downloaded a copy of the test database from Gitlab where it's hosted. I look forward to your input. I'm 100% sure there is no need for a full database.

The Feature tests are old and manky, we can focus on API and Unit tests for now.

As such I'm not a big fan of getting sucked into the GitHub sphere even further and I have a preference for Travis CI and things I could run locally. But there's little rocket science in these yaml files.

I'll do a deepdive over the weekend and get back to you. Feel free to tinker with your PR and tell me what to do. There's a chance I'll copy your code and submit it myself to see what happens, step by step. But I'll reverse it all and merge to make sure you get the credits.

Thanks again for your efforts! 🎉

@JC5 JC5 merged commit 6e6dd60 into firefly-iii:develop Jul 31, 2020
@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2020

SonarCloud Quality Gate failed.

Bug C 205 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 56 Code Smells

No Coverage information No Coverage information
7.9% 7.9% Duplication

@hoshsadiq
Copy link
Contributor Author

I'm a stickler for the name, it's "Firefly III", not "Firefly".

Ah pardon me, will keep that in mind in the future.

I'm happy to see what PHPStan and other static analysers come up with (SonarCloud is also installed) but I couldn't really care less about their results. 8000 positive results is a sign of 7000 false positives and duplicates, if PHPStan is anything like any static analyser out there.

Totally agree. In my mind the plan was to see what it comes back with, and then add whatever false positives to the ignore list, and then only fix we deemed important.

I have a similar opinion about code standards. My own PHPStorm installation is pretty strict already. I'm not going to fight a tool over where my brackets are. But I don't mind being told about missing docblocks.

I guess the problem isn't necessary about you, it's for others committing. I personally see phpstorm's defaults as a good standard, but not everyone uses phpstorm. The alternative is that whoever maintains the code (which in this case is you) would have to go in and ensure standards are followed post merge.

As for the whole thing about fixtures. I currently run the tests locally in conjunction with a script that generates a fresh database every time. It's what powers the demo site too; it generates new and up-to-date transactions daily. I used to ship a test database as well but the last time I did CI using Travis CI I downloaded a copy of the test database from Gitlab where it's hosted. I look forward to your input. I'm 100% sure there is no need for a full database.

Strange! I ran it locally, and it refused to run without the users. I just didn't have time to actually figure out the code to add the user. Maybe I overlooked something

As such I'm not a big fan of getting sucked into the GitHub sphere even further and I have a preference for Travis CI and things I could run locally. But there's little rocket science in these yaml files.

Totally understand that, I'd be happy to change it to travis, or even circle (which officially supports running your configs locally).

Lastly, the PR wasn't really ready. Was there a reason you merged?

@JC5
Copy link
Member

JC5 commented Jul 31, 2020

Yeah I wanted to see what would happen, and you didn't put it on draft status. Looking at GitHub feature set when it comes to actions, I'm not overly impressed I must say. My goal for this evening was to rejuvinate the Travis CI build

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
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.

None yet

2 participants