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

Making tests work #100

Merged
merged 10 commits into from Apr 2, 2022
Merged

Making tests work #100

merged 10 commits into from Apr 2, 2022

Conversation

lucasjose501
Copy link
Contributor

This PR prepares the server code for testing and future development. Also fixing current tests.

You can test the application using the console command 'php artisan test'. Current code coverage is 53% and I'll be working on this soon.
Some tests are skipped because they are not implemented yet.

@lucasjose501
Copy link
Contributor Author

Now that I saw #99 I realized that the forgot-password tests can not be implemented, and I'll remove the file completely because it's not necessary.

@bimbashrestha bimbashrestha self-requested a review March 9, 2022 19:11
@bimbashrestha
Copy link
Member

Hey @lucasjose501 at a quick glance, the changes look good. Thanks for making the PR!

I'm realizing I need to sit down and think a bit more carefully about how to test Blobbackup. I'll come back to this once I've done that. Sorry for the delay btw.

@lucasjose501
Copy link
Contributor Author

Hi @bimbashrestha
I have some projects with automated tests on github that I can show the script I'm using for you. It is straight forward and with little modification for your needs it can be activated here. But I think that separated repos could make life easier.

@lucasjose501
Copy link
Contributor Author

Here an example: https://gist.github.com/lucasjose501/e1a561f5b4ce4e66bf2ac65a896ee824

'email' => $this->faker->unique()->safeEmail(),
'email_verified_at' => now(),
'password' => '$2y$10$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi', // password
'password' => bcrypt('password'),
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@bimbashrestha bimbashrestha left a comment

Choose a reason for hiding this comment

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

Hey @lucasjose501 sorry for the delay in reviewing this! Was a little caught up with some other tasks.

The changes look great! I tested your PR and I see that ./vendor/bin/phpunit passes now:)

Here an example: https://gist.github.com/lucasjose501/e1a561f5b4ce4e66bf2ac65a896ee824

I like this workflow. Do you mind including your yml file inside the .github/workflows folder in this PR? I'd suggest having tests run on push and pull_request to the dev branch for the time being.

Everything else looks good to me. I'll merge after we add a yml file.

@lucasjose501
Copy link
Contributor Author

lucasjose501 commented Apr 2, 2022

Not sure if this will work now because of the folder structure in the repo.

Edit: Also, I'm not expert in PRs on github but I'll make future PRs in separated branches instead of dev, sorry for that.

@bimbashrestha
Copy link
Member

Not sure if this will work now because of the folder structure in the repo.

Looks like it worked: https://github.com/Blobbackup/Blobbackup/runs/5797062568?check_suite_focus=true. Nicely done!

Edit: Also, I'm not expert in PRs on github but I'll make future PRs in separated branches instead of dev, sorry for that.

I think it's perfectly fine to make PRs directly to dev for changes that are unlikely to affect the current production service (testing qualifies!).

Thanks @lucasjose501!

@bimbashrestha bimbashrestha merged commit 38f4a8d into Blobbackup:dev Apr 2, 2022
@lucasjose501
Copy link
Contributor Author

That was smooth haha, I hope I got it right and will trigger only for changes in the 'server' directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants