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

Add another test for fork within fork wrapped in join #73

Merged
merged 11 commits into from Aug 23, 2017
Merged

Conversation

@tiagofilipe12
Copy link
Member

@tiagofilipe12 tiagofilipe12 commented Aug 17, 2017

This PR fixes an issue with fork inside fork. Avoids the duplication of outermost tasks when fork is not nested in a join inside a fork. Previously this was not being properly handled because there was no way to check whether outermost tasks where the same as downstream tasks.

The rational is when fork is inside fork without join wrapping, outermost tasks will be the same as downstream tasks and thus should not be added to lineage twice. On the other hand when join is wrapping the inner fork, outermost tasks (after outer fork) are different from downstream tasks (after inner fork) and thus both should be added to lineage.

To achieve the comparison of these two arrays (outermostTasks and downstreamTasks) I have made two hashes for each by adding the individual hashes of each task within each one of these arrays, where these hashes are made with task.info.operationCreator. So, when all tasks inside both arrays have the same operationCreator it will return the same hash, otherwise they will be different.

Then was just a matter of checking if both hashes are equal or not.

@tiagofilipe12 tiagofilipe12 self-assigned this Aug 17, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Aug 17, 2017

Codecov Report

Merging #73 into dev will increase coverage by 0.43%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #73      +/-   ##
==========================================
+ Coverage   81.83%   82.27%   +0.43%     
==========================================
  Files          37       37              
  Lines         881      897      +16     
  Branches      107      108       +1     
==========================================
+ Hits          721      738      +17     
+ Misses        160      159       -1
Impacted Files Coverage Δ
lib/orchestrators/join.js 92.55% <85.71%> (+0.24%) ⬆️
lib/reducers/collection.js 87.77% <0%> (+2.22%) ⬆️

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 8fa4702...f1489b9. Read the comment docs.

Loading

@tiagofilipe12 tiagofilipe12 requested a review from thejmazz Aug 18, 2017
@tiagofilipe12 tiagofilipe12 force-pushed the orchestrators_tests branch from 52905bb to d76bb9f Aug 21, 2017
@tiagofilipe12
Copy link
Member Author

@tiagofilipe12 tiagofilipe12 commented Aug 21, 2017

multiple forks inside other forks were also fixed in this commit.

Loading

@tiagofilipe12
Copy link
Member Author

@tiagofilipe12 tiagofilipe12 commented Aug 21, 2017

Tests for junction inside fork and fork inside fork (either wrapped in join or not) were added to check the shape of graph. This was performed by counting the length of the graphson array for vertices and for edges. I am aware that this is far from ideal but it is a quick way to check if something is broken in the near future. These tests must be re-worked after refactoring the pipeline object to something that can be run before execution.

Loading

@bmpvieira bmpvieira added this to In Progress in Bionode Project Board Aug 22, 2017
@thejmazz thejmazz merged commit 2a68747 into dev Aug 23, 2017
5 checks passed
Loading
@thejmazz
Copy link
Member

@thejmazz thejmazz commented Aug 23, 2017

@tiagofilipe12 delete branch?

Loading

@thejmazz thejmazz moved this from In Progress to Done in Bionode Project Board Aug 23, 2017
@tiagofilipe12 tiagofilipe12 deleted the orchestrators_tests branch Aug 23, 2017
@tiagofilipe12 tiagofilipe12 mentioned this pull request Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants