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

Refactor selection box layer - Pass 1 #2667

Merged
merged 6 commits into from
Aug 25, 2015
Merged

Conversation

aicioara
Copy link
Contributor

Part of #2623

I will be doing 2 passes because I will get more context (and better sense of best practices) by refactoring other files. However, please flag anything that seems unusual.

As part of this:

  • Test semantics: hierarchy, naming, testing, but no coverage check
  • Applied best practices from the wiki page
  • Factored out common code into beforeEach() clauses
  • No nested beforeEach()es
  • Remade hierarchy as shown bellow
    screen shot 2015-08-24 at 12 59 14 pm

@aicioara aicioara changed the title Refactor selection box layer Refactor selection box layer - Part 1 Aug 24, 2015
@aicioara aicioara changed the title Refactor selection box layer - Part 1 Refactor selection box layer - Pass 1 Aug 24, 2015
assert.strictEqual(sbl.boxVisible(), false, "Setting the boxVisible attribute to false works");

svg.remove();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This crosses a very fine line of testing, but I have thinking about how to effectively test getters... only to end with the conclusion of not to test them directly (The best way to test them directly is to pull the corresponding variable backing the getter but then this leads into all sorts of trouble).

The best thing I can think of is to utilize the getters as our means of detecting correctness for the rest of the code (If we use a setter, then the getter should definitely reflect the set value) (If a state-modifying function is called, the getter should definitely reflect the modified state).

Sure we can use these unit tests to prove that the function exists, but I am unsure of its usefulness beyond that (Also this specific idea would be not useful for typescript consumers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @jtlan was saying. We have to build up our tests on axioms. This tests proves that the getter and the setter are having the expected behavior, so we can use them later assuming they are correct.

I must agree that the usefulness of this test is below the average though.

@bluong
Copy link
Contributor

bluong commented Aug 25, 2015

A few suggestions

@bluong bluong added the -1 label Aug 25, 2015
@bluong bluong assigned aicioara and unassigned bluong Aug 25, 2015
@aicioara
Copy link
Contributor Author

I liked the idea of destroying the components, hence I applied it. Not sure what you mean about the getters / setters

@aicioara aicioara removed the -1 label Aug 25, 2015
@aicioara aicioara assigned bluong and unassigned aicioara Aug 25, 2015
@bluong
Copy link
Contributor

bluong commented Aug 25, 2015

I think having tests to test the setters is perfectly valid, but I do not think it makes sense for us to explicitly say that we are testing the getters.

Thus, in this scenario, I would say to change the test names to just saying that you can use the setters and then inside the test just use the getters to get at the set values.

Something like
it("can set if the box is visible")

@aicioara
Copy link
Contributor Author

Sounds good

@aicioara aicioara assigned aicioara and unassigned bluong Aug 25, 2015
@aicioara aicioara added the -1 label Aug 25, 2015
@aicioara aicioara removed the -1 label Aug 25, 2015
@aicioara aicioara assigned bluong and unassigned aicioara Aug 25, 2015
@bluong bluong added the +1 label Aug 25, 2015
@bluong bluong assigned aicioara and unassigned bluong Aug 25, 2015
aicioara added a commit that referenced this pull request Aug 25, 2015
Refactor selection box layer tests - Pass 1
@aicioara aicioara merged commit c31bf2b into develop Aug 25, 2015
@aicioara aicioara deleted the refactorSelectionBoxLayer branch August 25, 2015 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants