Skip to content

Conversation

@mplieske
Copy link
Contributor

No description provided.

</project>

<project>
<name>Carl Zeiss</name>
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use some other project name

final String isWork = eElement.getElementsByTagName("isWork").item(0).getTextContent();
final String color = eElement.getElementsByTagName("color").item(0).getTextContent();

System.out.println("checking for: " + name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use a logger instead for sysout

public class ConfigParser {

Controller controller;
Model model;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't use the model in the class - delete it

private final ObservableList<Project> projects = FXCollections.observableArrayList();

Model model = new Model();
private final ObservableList<Project> spyModel = spy(model.availableProjects);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice :) But as we mock the Controller, we don't have to test if the controller writes the new projects into the model correctly. We should just make sure, that controller.addNewProject is called as often as we expect with the needed parameters So we could get rid of all references of the model.

return f.exists();
}

public void parserConfig(final File inputFile) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could be renamed to parseConfig


loadConfigFile(CONFIG_FILENAME);

verify(controller).addNewProject(eq("Carl Zeiss"), anyBoolean(), any(Color.class), anyInt());
Copy link
Collaborator

Choose a reason for hiding this comment

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

there you should verify if the isWork, color, index are the ones you expect the loader to create (like with the name)


loadConfigFile(CONFIG_FILENAME);

verify(controller, atLeastOnce()).addNewProject(eq("Peter Lustig"), anyBoolean(), any(Color.class), anyInt());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this would return true, if the addNewProjects was not called in this test, as it was called in another test before. As you use the same controller instance for all tests? I think you should create a new mock-controller for each test case e.g. with @before

private final ObservableList<Project> spyModel = spy(model.availableProjects);
private final Controller controller = mock(Controller.class);

@Before
Copy link
Collaborator

Choose a reason for hiding this comment

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

before is run before each single test. so after n tests there should be n*4 projects in the projects list. I guess that is not what you wanted :) beforeclass can be used in this case, where the method is called just once before all tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, junit '@test' annotation instatiates a new Object of your test class for each test :)

@Death111
Copy link
Collaborator

Also there should be some mechanism to ask the user if he wants to import the config, but not each time he starts the app :)

@Death111 Death111 merged commit 635743e into doubleSlashde:master Sep 24, 2018
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.

2 participants