Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Added tests for contrib.locked_file. #467

Merged
merged 1 commit into from
Mar 18, 2016

Conversation

theacodes
Copy link
Contributor

Towards #212

This module beastly. It's only user is multistore_file and honestly I would be in favor of outright removing this and that from the library altogether.

These tests do not cover the platform-specific opener classes - I'd really like to hear some ideas on how to test those.

@theacodes
Copy link
Contributor Author

@thobrla it seems that gsutil is a user of multistore_file here. How would you feel about us trying to remove this? It's a lot of code (and cruft) for a very specific use case. Is there a better storage option we can provide (sqlite?)

@thobrla
Copy link
Contributor

thobrla commented Mar 17, 2016

I'm fine with removing both locked_file/multistore_file if a suitable alternative is provided. The requirement is to safely use a single credential across multiple processes and threads. If we can find a better cross-platform way to do that, I'm happy to port gsutil over (other incompatibilities notwithstanding).

@theacodes
Copy link
Contributor Author

I'll start a separate issue for that, thanks, @thobrla.

instance._locked = True
self.assertTrue(instance.is_locked())

def test_is_locked(self):

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Mar 18, 2016

LGTM mostly

@theacodes
Copy link
Contributor Author

Updated, should address most of your comments.

Thanks for the review. I have no idea what it is about this library but I just end up writing sloppy tests. :|

@dhermes
Copy link
Contributor

dhermes commented Mar 18, 2016

LGTM more mostly

@theacodes
Copy link
Contributor Author

Any qualms with merging this? Should we get @nathanielmanistaatgoogle? I'm just past caring on this one and would rather push for its removal if we can get gcloud cli and gsutil to play along.

@dhermes
Copy link
Contributor

dhermes commented Mar 18, 2016

Ha sorry my last comment was meant to be a "go-ahead and merge" LGTM

@theacodes
Copy link
Contributor Author

Ha. Fair enough. I'm still open to suggestions on testing the platform-specific stuff (mocks?) provided we don't vote it off the island.

theacodes pushed a commit that referenced this pull request Mar 18, 2016
Added tests for contrib.locked_file.
@theacodes theacodes merged commit 3ca2ca7 into googleapis:master Mar 18, 2016
@theacodes theacodes deleted the coverage-locked-file branch March 18, 2016 18:13
@theacodes theacodes mentioned this pull request Apr 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants