Skip to content
This repository has been archived by the owner on Jan 16, 2023. It is now read-only.

Set the test suite regardless of actual cloud connection. #218

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mode9
Copy link
Collaborator

@mode9 mode9 commented Jan 16, 2022

Now we can run the test suite independently without the actual access key of AWS and GCP.

  • mock boto3.s3 before starting tests with moto
  • mock GoogleCloud Client, Bucket, Blob classes.

@codeclimate
Copy link

codeclimate bot commented Jan 16, 2022

Code Climate has analyzed commit e77d23b and detected 0 issues on this pull request.

View more on Code Climate.

@mode9 mode9 closed this Jan 16, 2022
@mode9 mode9 changed the title Set the test suite regardless of actual cloud connection. [Duplicate] Set the test suite regardless of actual cloud connection. Jan 16, 2022
@mode9 mode9 reopened this Jan 16, 2022
@mode9 mode9 changed the title [Duplicate] Set the test suite regardless of actual cloud connection. Set the test suite regardless of actual cloud connection. Jan 16, 2022
@mode9
Copy link
Collaborator Author

mode9 commented Jan 16, 2022

Send Code Climate Coverage failing is about CC_TEST_REPORTER_ID.

Copy link
Owner

@antonagestam antonagestam left a comment

Choose a reason for hiding this comment

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

I'm not sure this PR gets it all the way? The goal here should be to remove all distinctions between "live" and "non-live" tests, so that the entire test suite can run without making any real connections to backend services.

I'm not seeing anything that verifies that connections to GCP are in correct shapes, we need to have that in place so that we can safely refactor that code. That currently relies on the "live" tests failing on real connections to GCP.

The Github Actions workflow needs to be updated to remove the SKIP_LIVE_TESTS conditionals, and probably all traces of SKIP_LIVE_TESTS and the @live_test should be removed too.

AWS_S3_SIGNATURE_VERSION = "s3v4"
AWS_QUERYSTRING_AUTH = False
AWS_DEFAULT_ACL = None
S3_USE_SIGV4 = True
AWS_S3_HOST = "s3.eu-central-1.amazonaws.com"
AWS_S3_HOST = "s3.ap-northeast-2.amazonaws.com"
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any reason to change this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, just for live testing with my accounts.
I'll revert it.

@@ -3,14 +3,13 @@

from django.test import override_settings as override_django_settings

from collectfast.tests.command.utils import call_collectstatic
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seem like a justified change, let's keep this a relative import.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought the absolute path was more straightforward. But in this case, I guess relative is enough. 😄

@@ -5,6 +5,7 @@
from django.test import override_settings as override_django_settings

from collectfast.management.commands.collectstatic import Command
from collectfast.tests.command.utils import call_collectstatic
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, let's restore this import.

Comment on lines 15 to 16
import boto3 # type: ignore
import moto # type: ignore
Copy link
Owner

Choose a reason for hiding this comment

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

Let's install types for boto3, and ignore moto in setup.cfg.

Related: pytest should be possible to remove from setup.cfg since it has gained typing support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds great.

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.

2 participants