lonnieezell / Bonfire2

CodeIgniter 4-based application skeleton
MIT License
130 stars 50 forks source link

Fixes for menu and login tests #386

Closed dgvirtual closed 1 year ago

dgvirtual commented 1 year ago

I have looked into the phpunit tests that were failing and found that perhaps I could actually fix them (and make some adjustments in the code). Now phpunit tests pass on my computer.

dgvirtual commented 1 year ago

Well, my fixed tests failed the tests. I did actually run the composer update on php 8.1.18 before fixing tests... I have also switchet to php7.4 and phpunit ran tests fine, but I did not rerun composer update after that.

Hope that helps in troubleshooting.

dgvirtual commented 1 year ago

So I took a deep breath and submitted updated composer.lock (run composer update on php 7.4.33 version). All tests pass, I see!

datamweb commented 1 year ago

So I took a deep breath and submitted updated composer.lock (run composer update on php 7.4.33 version). All tests pass, I see!

@dgvirtual One thing at a time: A pull request should only contain one change. That does not mean only one commit, but one change - however many commits it took. The reason for this is that if you change X and Y, but send a pull request for both at the same time, we might really want X but disagree with Y, meaning we cannot merge the request. Using the Git-Flow branching model you can create new branches for both of these features and send two requests.

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#branching

I mean do the action error fix in a separate PR. This makes the review and merge to be done faster.

dgvirtual commented 1 year ago

Thanks @datamweb, I will try to figure out how to remove the composer.lock from this PR and create a different one with it.

datamweb commented 1 year ago

The following command takes a backup of your branch.

git branch fixes-for-menu-and-login-tests.bk fixes-for-menu-and-login-tests

To delete the commit related to file changes "composer.lock":

git reset --hard 3c26c69
git push -f origin fixes-for-menu-and-login-tests
dgvirtual commented 1 year ago
git reset --hard 3c26c69

This, I guess, had to refer to a commit a step earlier. Otherwise I did as suggested, and submitted another PR with the composer.lock update. Thanks for the tips!

dgvirtual commented 1 year ago

@datamweb, could you take a look at this collection of commits. They sort of belong to the same category (enabling and improving mobile view for Bonfire2). If I submit them as separate PRs, I am afraid it would look like flooding... Could that count as one PR?

datamweb commented 1 year ago

@dgvirtual I don't use BF2. That's why I don't have full command of the code. The items in the link are understandable, I think there is no problem.

In general, try to create small PR as much as possible, this makes it easier for the Maintainer to review, if the PR are large, the Maintainer may pass it due to lack of time.

For this particular case, for example, you can attach screenshots before and after in PR description.

lonnieezell commented 1 year ago

Looks good. Thanks @dgvirtual and huge thanks to @datamweb for the advice offered!