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

[WIP] Test for merge function #16

Merged

Conversation

peterclemenko
Copy link

This is a test and RFC for merge functionality. Do not merge this yet, as it is untested.

@LucianoPAlmeida LucianoPAlmeida changed the title test for merge function [WIP] Test for merge function Mar 25, 2018
let node2 = nodes[1];
let rel1 = OGMNeoRelation.relateMerge(node1.id, 'relatedto', node2.id, { property: 'a' });
let rel2 = OGMNeoRelation.relateMerge(node1.id, 'relatedto', node2.id, {});
let rel3 = OGMNeoRelation.relateMerge(node1.id, 'relatedto', node2.id, { property: 'a' });
Copy link
Owner

Choose a reason for hiding this comment

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

rel3 and rel4 aren't being used here on this test

Copy link
Author

Choose a reason for hiding this comment

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

Alright I forgot about this. I'll fix it shortly.

Copy link
Owner

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

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

Hey @AoiGhost I think it's good, post some comments above. Just some linting problems and some tests that actually test node counts that fail because they are being affected by the new tests that create a new node.

@peterclemenko
Copy link
Author

Alright, I forgot about this. I'm probably going to add create unique as well in an updated version of this, hang tight.

@codecov-io
Copy link

codecov-io commented Jun 28, 2018

Codecov Report

Merging #16 into merge-function will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           merge-function      #16      +/-   ##
==================================================
+ Coverage           98.06%   98.14%   +0.08%     
==================================================
  Files                  21       21              
  Lines                1857     1940      +83     
  Branches              187      191       +4     
==================================================
+ Hits                 1821     1904      +83     
  Misses                 36       36
Impacted Files Coverage Δ
__test__/relation-tests.js 100% <100%> (ø) ⬆️
lib/ogmneo-node.js 96.87% <100%> (+0.17%) ⬆️
__test__/node-tests.js 100% <100%> (ø) ⬆️
lib/ogmneo-relation.js 97.72% <100%> (+0.24%) ⬆️

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 5caf6e2...ee9ea7f. Read the comment docs.

@peterclemenko
Copy link
Author

Reading up again, create unique is deprecated. Never mind, I'll just go with merge.

@peterclemenko
Copy link
Author

OK, from the looks of it my code is fixed and hopefully should be mergeable @LucianoPAlmeida. That being said, looks like travis is finding test issues in the base code, so you might want to take a look.

@LucianoPAlmeida
Copy link
Owner

LucianoPAlmeida commented Jun 29, 2018

Hey @AoiGhost
Thank's for that :))
I think the Travis error is just in some tests that return the nodes counts and since you create new nodes on the merge tests it was expecting like one and got two. There are a few solutions to that:

  • Update tests 'Test updateMany node' and 'Test delete MANY NODE' expectations to + 1.
  • Delete the nodes created on the test merge after the test completes.
  • Or just use another label for the nodes on the merge tests that you implemented.

That's actually not the good scenario for unit tests to impact each other, my bad on that. Can you make this changes? Any of those are acceptable, I'll probably decouple the unit tests later so they don't depend on each other.
After that, I think it will be good to go :))

@LucianoPAlmeida LucianoPAlmeida changed the base branch from master to merge-function August 12, 2018 04:05
@LucianoPAlmeida LucianoPAlmeida merged commit 36e47e3 into LucianoPAlmeida:merge-function Aug 12, 2018
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

3 participants