Skip to content

Conversation

e412
Copy link
Collaborator

@e412 e412 commented Dec 18, 2019

Test funktionieren noch nicht (siehe Email)

Copy link
Collaborator

@NoraAnlima NoraAnlima left a comment

Choose a reason for hiding this comment

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

Sorry für die späte Review but here it is now 😃.

Comment on lines +7 to +10
snaps:
- docker
services:
- neo4j
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also wir glauben, dass docker als snap laufen zu lassen nicht unbedingt der beste Ansatz ist (eine Containerumgebung in einer Sandbox hört sich nach Problemen an).

Ihr könnt da auch direkt als Service docker angeben und dann einfach in before_install den offiziellen Neo4j Container hochfahren. Siehe unsere travis.yml bei Interesse

Vielleicht löst das dann auch euer Travis Problem ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wir hatten Probleme damit, dass unser Driver nicht geschlossen wurde und deswegen die Tests endlos liefen. Um das zu lösen, haben wir für den Server eine Cleanup-Funktion implementiert, die bei dessen stop event aufgerufen wird. Leider hat das aber für den Test nichts gebracht, weil der Mockup Server dieses event anscheinend nicht aufruft, weshalb wir die Funktion jetzt immer manuell in afterAll() aufrufen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Travis CI unterstützt docker: https://docs.travis-ci.com/user/docker/ bei uns nutzen wir docker um die gebauten images zu testen. Docker auf Travis hat natürlich einen Nachteil weil der build server erstmal die ganzen images bauen muss was entsprechend Zeit verbraucht. Jetzt hier bei der Aufgabe werden keine Images gebaut, allerdings ist das caching von npm modules nicht so ohne weiteres möglich.

Die Lösung, den driver im beforeAll zu schließen klingt ganz sinnvoll. Alternativ könnt ihr auch globalTeardown nutzen: https://jestjs.io/docs/en/configuration#globalteardown-string

"dev": "cross-env NODE_ENV=development webpack-dev-server --open --hot",
"build": "cross-env NODE_ENV=production webpack --progress --hide-modules",
"test": "jest"
"test": "jest --verbose -u --detectOpenHandles"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Das --detectOpenHandles haben wir auch ausprobiert. Das hat bei uns aber nur die Fehlermeldung versteckt (unsere Lösung siehe oben). Hat das bei euch mehr gemacht? 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gut zu wissen! Also ich glaube, wir machen das bei HC auch falsch, weil wir einfach--forceExit benutzen. Vermutlich haben wir das gleiche Problem, also dass der Neo4j driver nicht geschlossen wird.

Comment on lines +7 to +10
await session.run(
'CREATE (:User {id: $id, name: $name, email: $email, password: $password})',
user
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Das muss in einen try Block falls da eine Exception fliegt. :)

'CREATE (:User {id: $id, name: $name, email: $email, password: $password})',
user
);
await session.close();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Und das dann in einen finally-Block um die Ressourcen wieder freizugeben.

Copy link
Owner

Choose a reason for hiding this comment

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

ja das ist natürlich eine gute Idee :D

Copy link
Collaborator

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

Alright, looks good!

alright

You still seem to have troubles with your software tests, shall we do another pair-programming maybe? Would that help?

Also I don't see a type resolver. If you return a type Todo let's say in a CreateTodo mutation. If this Todo has an assignee: User then you can request nested data:

mutation CreateTodo {
  task
  assignee {
    name
  }
}

So, resolving the nested data is better off in a type resolver, because you can implement it only once. Did you check if your backend crashes if you request nested data?

Comment on lines +7 to +10
snaps:
- docker
services:
- neo4j
Copy link
Collaborator

Choose a reason for hiding this comment

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

Travis CI unterstützt docker: https://docs.travis-ci.com/user/docker/ bei uns nutzen wir docker um die gebauten images zu testen. Docker auf Travis hat natürlich einen Nachteil weil der build server erstmal die ganzen images bauen muss was entsprechend Zeit verbraucht. Jetzt hier bei der Aufgabe werden keine Images gebaut, allerdings ist das caching von npm modules nicht so ohne weiteres möglich.

Die Lösung, den driver im beforeAll zu schließen klingt ganz sinnvoll. Alternativ könnt ihr auch globalTeardown nutzen: https://jestjs.io/docs/en/configuration#globalteardown-string


const server = new ApolloServer({ typeDefs, resolvers});
if (process.argv.length === 3 && process.argv[2] === "--seed") {
seedDatabase(driver)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer a dedicated npm script, I think it's more flexible and simpler, but up to you.

"dev": "cross-env NODE_ENV=development webpack-dev-server --open --hot",
"build": "cross-env NODE_ENV=production webpack --progress --hide-modules",
"test": "jest"
"test": "jest --verbose -u --detectOpenHandles"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Gut zu wissen! Also ich glaube, wir machen das bei HC auch falsch, weil wir einfach--forceExit benutzen. Vermutlich haben wir das gleiche Problem, also dass der Neo4j driver nicht geschlossen wird.

]

module.exports.todos = todos;
module.exports.users = users; No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

I put in this PR for another group to show you how to setup babel. m-oehme#6 With babel you can use import and export statements.

}
});
expect(res.data.deleteTodo.toBeUndefined);
expect(res.data.deleteTodo).toBeNull();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please expect the res.errors here.

password: "password"
}
});
expect(res.data.signup).toBeDefined();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to see the servers response here:

await expect(mutate({
  mutation: SIGNUP,
  variables: {
      name: "eva",
      email: "[email protected]",
      password: "password"
  }
})).resolves.toMatchObject({
  data: {
    signup: {
      // expect your data here
    }
  },
  errors: undefined, // no errors
})

});

const GET_ALL_USERS = gql `
query {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix the indentation here, use graphql-playground and click on "Prettify".

const bcrypt = require('bcryptjs')
const jwt = require('jsonwebtoken')
const {
neo4jgraphql
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this import used? I searched for neo4jgraphql and couldn't find where you use it.

describe('query', () => {
describe('todos', () => {
it('todo_list has 2 initial todos', async () => {
it('querying all todos returns only 1 Todo because of limit', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
it('querying all todos returns only 1 Todo because of limit', async () => {
it('returns only 1 Todo because of limit', async () => {

"it returns only 1 Todo because of limit" is more like an English sentence than "it querying all todos returns only 1 Todo because of limit"

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