kitovu-bot / kitovu

OpenHSR Connect 2
GNU General Public License v3.0
5 stars 2 forks source link

Initial syncing prototype #1

Closed The-Compiler closed 6 years ago

The-Compiler commented 6 years ago

Synchronizes a single file via SMB. Still some FIXMEs here and there, but should be ready to merge and improve upon.

codecov[bot] commented 6 years ago

Codecov Report

Merging #1 into master will increase coverage by 6.66%. The diff coverage is 87.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
+ Coverage      80%   86.66%   +6.66%     
==========================================
  Files           4       13       +9     
  Lines          15      270     +255     
  Branches        1       13      +12     
==========================================
+ Hits           12      234     +222     
- Misses          3       35      +32     
- Partials        0        1       +1
Impacted Files Coverage Δ
src/kitovu/cli.py 0% <0%> (-100%) :arrow_down:
src/kitovu/__main__.py 0% <0%> (ø) :arrow_up:
tests/sync/test_syncing.py 100% <100%> (ø)
tests/test_kitovu.py 100% <100%> (ø) :arrow_up:
src/kitovu/utils.py 100% <100%> (ø)
tests/sync/test_smb.py 100% <100%> (ø)
tests/conftest.py 100% <100%> (ø)
src/kitovu/config.py 100% <100%> (ø)
src/kitovu/sync/syncplugin.py 100% <100%> (ø)
src/kitovu/sync/syncing.py 57.57% <57.57%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 44ace40...528f540. Read the comment docs.

ThunderKey commented 6 years ago

@The-Compiler according to the documentation of the SMB URL the authdomain should be infront of the username. I've corrected it in ec383f48b16ccaf2fe2c96e3c5655d3abf12d973

ThunderKey commented 6 years ago

Travis seems to have trouble with the keyrings. What about adding the keyrings.alt package? It's not secure, but that is fine for testing purposes

The-Compiler commented 6 years ago

Good catch with the SMB URL!

I rewrote some of your code in 9b73961b1e688ccb80e709498ae81d2bcad6d470 to use pytest fixtures, especially monkeypatch (which does a lot less magic than unittest.mock and is preferable for simple cases like this).

Travis seems to have trouble with the keyrings. What about adding the keyrings.alt package? It's not secure, but that is fine for testing purposes

That test is currently flawed anyways - it'll modify your system's keyring and ask for the password when running the testsuite (if it's not unlocked yet). Instead, the test should probably set a test backend for keyring: https://github.com/jaraco/keyring#write-your-own-keyring-backend - do you want to take a look at this or should I?

ThunderKey commented 6 years ago

Thanks for the improvements!

That is a good idea with the test backend. It seems straight forward, I'll give it a go this weekend.

ThunderKey commented 6 years ago

I've added in 1fde7245328271995cd86de11db4d7152b004ce9 a InMemoryKeyring for the tests which gets cleared before each test.

I'm not sure if the folder /tests/helpers is the best location for support/helper files. Is there a prefered location?

The-Compiler commented 6 years ago

@ThunderKey I did some changes to your test code in 528f5400701e0afbbd6b989c98b012d58f0d9224 - quick overview:

Let me know if you disagree with any of that, or if anything seems unclear! Merging this now so we can proceed with new PRs for future changes.