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

Remove the confusing "dummy_test_module" directory #1756

Merged
merged 1 commit into from
Jul 14, 2016

Conversation

Tarrasch
Copy link
Contributor

Description

Remove the confusing "dummy_test_module" directory

Motivation and Context

The purpose is simply to remove a confusing module from the luigi root
directory. It was only used from tests but it couldn't be put in the
test module for whatever technical reasons.

Have you tested this? If so, how?

This is only affecting tests. If tests pass it should be OK.

@mention-bot
Copy link

@Tarrasch, thanks for your PR! By analyzing the annotation information on this pull request, we identified @daveFNbuck, @erikbern and @themalkolm to be potential reviewers

The purpose is simply to remove a confusing module from the luigi root
directory. It was only used from tests but it couldn't be put in the
test module for whatever technical reasons.
@codecov-io
Copy link

Current coverage is 76.15%

Merging #1756 into master will decrease coverage by 2.65%

@@             master      #1756   diff @@
==========================================
  Files            95         95          
  Lines         10350      10350          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           8157       7882   -275   
- Misses         2193       2468   +275   
  Partials          0          0          

Powered by Codecov. Last updated by b98898b...e1baf1c

@Tarrasch
Copy link
Contributor Author

@daveFNbuck, does this look sane to you?

@daveFNbuck
Copy link
Contributor

I'm not sure it makes sense to import the unloaded module for the dynamic
loading test. I think importing it invalidates the test.

On Wed, Jul 13, 2016 at 9:43 PM, Arash Rouhani notifications@github.com
wrote:

@daveFNbuck https://github.com/daveFNbuck, does this look sane to you?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1756 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AB_rbenwL_J22LgnO2_JN9JIPm5tBEi_ks5qVb5XgaJpZM4JJHbp
.

@Tarrasch
Copy link
Contributor Author

I'm not sure it makes sense to import the unloaded module for the dynamic loading test. I think importing it invalidates the test.

Not sure we're on the same page.

The helper creates a module (a file within our PYTHONPATH) without loading it. Later, we run luigi --module that_yet_unloaded_module ..., and see if luigi is actually loading it or not.

@daveFNbuck
Copy link
Contributor

Sounds good.

@Tarrasch Tarrasch merged commit 5f1c5d3 into spotify:master Jul 14, 2016
@Tarrasch Tarrasch deleted the remove-not-imported-file branch July 14, 2016 07:57
p7k pushed a commit to Celmatix/luigi that referenced this pull request Jul 28, 2016
* spotify/master: (24 commits)
  Add DateSecondParameter to parameter.py (spotify#1779)
  tox: Specify sphinx dependency better
  flake8: Unbreak travis build
  Excludes .tox from flake8 to prevent checking third-party libraries (spotify#1785)
  README: Remove monthly downloads badge
  Rename CentralPlannerScheduler to Scheduler (spotify#1781)
  Remove abstract Scheduler class (spotify#1778)
  Assistants: Don't affect longevity of tasks (spotify#1772)
  tests: Skip a inttermittently failing s3 test (spotify#1777)
  Update retcodes to handle new cases (spotify#1771)
  tests: Fix warning in remote_scheduler_test.py (spotify#1774)
  Remove sitecustomize file (spotify#1755)
  Fix exist method for ftp server
  Update copy() to return number and size of files copied
  Remove the confusing "dummy_test_module" directory (spotify#1756)
  Disable codecov comments on GitHub PRs (spotify#1754)
  Fix "owner_email" log message. (spotify#1762)
  docs: Install sphinx 1.4.4 in setup.py (spotify#1761)
  docs: Set minimum versions for sphinx (spotify#1760)
  Normalize ListParameter to be Immutable (spotify#1759)
  ...
Tarrasch added a commit that referenced this pull request Sep 6, 2016
This was just forgotten to be included in #1756
Tarrasch added a commit that referenced this pull request Sep 6, 2016
This was just forgotten to be included in #1756
This was referenced Jun 29, 2022
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.

4 participants