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

Make all tests offline #593 #1009

Merged
merged 2 commits into from
Jul 13, 2019
Merged

Make all tests offline #593 #1009

merged 2 commits into from
Jul 13, 2019

Conversation

athanikos
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 9, 2019

Codecov Report

Merging #1009 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1009   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          38      38           
  Lines        2293    2293           
======================================
  Hits         2293    2293

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 0c7e0c0...1b7c0d4. Read the comment docs.

@athanikos athanikos changed the title Make all tests offline #593 , reformatted code base to allow build to pass Make all tests offline #593 Jul 9, 2019
@athanikos
Copy link
Contributor Author

athanikos commented Jul 9, 2019

if this is ok then the "Make all tests offline #593 #1004 PR" should be closed.
This is formatted so that lint is passing and reuses the requirements file in the root folder as a parameter whereas the other one was not passing due to formatting and it used a requirements file created within the "tests/mockmodulers/connectors" folder.

@FabioRosado
Copy link
Member

Hello thank you for this, did you meant to create another PR for this issue?
I'm just checking with you to decide if I should close the other PR or not since I will not be able to merge a PR that has failing tests 👍

@athanikos
Copy link
Contributor Author

No I did not mean to do so. I am a rookie on this tool, so I am not sure why this happened.
You can close the other one (as you did) and keep this one for review.

@athanikos
Copy link
Contributor Author

Could anyone review this one and let me know if any issues?

@FabioRosado
Copy link
Member

Hey @athanikos apologies for not replying earlier I kind of forgot to review the PR i'm going to do that now 👍

@FabioRosado
Copy link
Member

I've just checked these changes and they look good. From your tests was this the only test that was causing issues when offline? Or there are more tests that would fail because no connection is available?

@athanikos athanikos closed this Jul 12, 2019
@athanikos athanikos reopened this Jul 12, 2019
@athanikos
Copy link
Contributor Author

athanikos commented Jul 12, 2019

Sorry accidentally closed it and reopened the PR.

I modified three methods :

  1. tests/test_loader.py::TestLoader::test_install_default_remote_module
    I mocked the git clone method which was calling the clone method to get the files from the internet. Then I added mockclone.side_effect = shutil.copy("requirements.txt", config["install_path"]) to copy requirements file (simulates installation by just copying that file)
  2. tests/test_loader.py::TestLoader::test_install_specific_local_git_module
  3. tests/test_loader.py::TestLoader::test_install_specific_local_path_module
    Those two methods had _install_module method twice in place . I replaced the first call in each method with with os.makedirs (simulates the installation of git modules) The second call to git clone was allready mocked.

Then I run all tests by disabling the wired connection and all three passed (previously failing when no internet connection )

@FabioRosado
Copy link
Member

I've just realised that, apologies I miss-read the changes.
I'm happy with this and I'm going to merge them into the core! Thank you so much for working on this issue and helping us with it 👍

We give stickers for new contributors if you would like to get some then please send your home address by DM to opsdroid twitter account 😄

@FabioRosado FabioRosado merged commit 0ed98b0 into opsdroid:master Jul 13, 2019
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