Supports multiple connections in the neo4j VSCode plugin #357
Conversation
|
| if (found) { | ||
| await found.notification.dismiss(); | ||
| return true; | ||
| } else { |
There was a problem hiding this comment.
We need this so we always close a notification when we've found it. Otherwise notifications keep piling up and if I connect to an instance and I disconnect, I would still be able to find the notification Connected to neo4j when that's not the state of the extension anymore
| const activityBar = workbench.getActivityBar(); | ||
| const maybeNeo4jTile = await activityBar.getViewControl('Neo4j'); | ||
| if (maybeNeo4jTile) { | ||
| neo4jTile = maybeNeo4jTile; | ||
| const connectionPannel = await neo4jTile.openView(); | ||
| const content = connectionPannel.getContent(); | ||
| const sections = await content.getSections(); | ||
| connectionSection = sections.at(0); | ||
| } | ||
| connectionSection = await getConnectionSection(workbench); | ||
| }); | ||
|
|
||
| async function clickOnContextMenuItem(item: string) { | ||
| const items = await connectionSection.getVisibleItems(); | ||
| await expect(items.length).toBeGreaterThan(0); | ||
| const connectionItem = items.at(0) as TreeItem; | ||
|
|
||
| const contextMenu = await connectionItem.openContextMenu(); | ||
| const menuItems = await contextMenu.getItems(); | ||
| const connect = menuItems.find(async (menuItem) => { | ||
| const menuText = await menuItem.elem.getText(); | ||
| return menuText === item; | ||
| }); | ||
|
|
||
| if (connect) { | ||
| const connectOption = await connect.elem; | ||
| await connectOption.click(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I just extracted this to common helpers
| await workbench.executeCommand(CONSTANTS.COMMANDS.RUN_CYPHER); | ||
| const webviews = await workbench.getAllWebviews(); | ||
| await expect(webviews.length).toBe(1); | ||
| test('results should depend on the instance we are connected to', async function () { |
There was a problem hiding this comment.
This is really the only new test from this file, the rest is restructuring
ncordon
force-pushed
the
multiple-connections
branch
from
0aa2a31 to
ad2131b
Compare
| NEO4J_SCHEME=neo4j | ||
| NEO4J_HOST=localhost | ||
| NEO4J_PORT={PORT} | ||
| NEO4J_USER=neo4j | ||
| NEO4J_DATABASE=neo4j | ||
| NEO4J_PASSWORD={PASSWORD} |
There was a problem hiding this comment.
I realized we don't need this file anymore because it was used to pass the port and password to the tests, but we can do that via a better mechanism
| extensionTestsEnv: { CYPHER_25: 'true' }, | ||
| extensionTestsEnv: { | ||
| CYPHER_25: 'true', | ||
| NEO4J_PORT: neo4jInstance.getMappedPort(7687).toString(), |
There was a problem hiding this comment.
This is the proper way to pass env variables to the tests
| function setSetting(file: string, variable: RegExp, value: string) { | ||
| const data = fs.readFileSync(file).toString(); | ||
| const result = data.replace(variable, value); | ||
| fs.writeFileSync(file, result); | ||
| } | ||
|
|
||
| function updateDotenvFile(port: number, password: string) { | ||
| const dotenvPath = path.join(__dirname, '../../tests/fixtures/'); | ||
| const dotenvTemplate = path.join(dotenvPath, '.env'); | ||
| const dotenvFile = path.join(dotenvPath, '.env.test'); | ||
| fs.copyFileSync(dotenvTemplate, dotenvFile); | ||
| setSetting(dotenvFile, /\{PORT\}/g, port.toString()); | ||
| setSetting(dotenvFile, /\{PASSWORD\}/g, password); | ||
| dotenv.config({ path: dotenvFile }); | ||
| } |
There was a problem hiding this comment.
Just cleans up these because we can pass the env variables better to the tests
There was a problem hiding this comment.
I had one question, otherwise looks good. I tested around a bit with it in the playground though, and found 2 things that were a bit unexpected.
1 - (and I think this one is how it was before too) - if I want to close down a "create new connection" pop-up, I dont have an x in the corner, or something like pressing esc or outside the pop-up to close it which is what I first expected. I then found that the pop-up actually opens like a new tab/file in the playground. If you agree that this is less intuitive (and its not too hard to change) maybe we should change it in a follow-up PR?
2 -
Creating new connections - to me it works like this:
if active db at given connection info (port etc) exists, creating connection with password x only works if x is the correct password. Too many tries gets your session blocked, You are also allowed to create multiple sessions with the exact same info (including password) this way
If db at connection info is not active, you get a pop-up ("Unable to connect...") and are allowed to add the connection.
The disparity of how this works is confusing to me, I would expect to either
A: never be able to add a db I can connect to, or
B: to always be allowed to add connections that dont exist at the moment.
(As a user, I would prefer option B, since I might want to create 3 dbs with same info but different passwords, and having to start/stop dbs between to create connections (instead of doing all at once) seems annoying)
Also once these connections are created, the order of creation and password length is the only way to tell them apart, so I could easily see myself forgetting which has which password. Possible solutions - nicknames for connections displayed next to the existing info. Or maybe we just dont allow multiple connections to same scheme/host/port/user, but I think I prefer the other one.
(edit: also another follow-up suggestion: since we don't show the password I assume we want it secret, so another step for that could be showing x amount of *s always, instead of having them match the password in length)
| const webviews = await workbench.getAllWebviews(); | ||
| await expect(webviews.length).toBe(1); | ||
| test('results should depend on the instance we are connected to', async function () { | ||
| if (os.platform() === 'darwin') { |
There was a problem hiding this comment.
From what I find online, "darwin" is a unix-variant used by Mac-OS, so would this test not run on Mac? Why if so?
There was a problem hiding this comment.
I guess because of the issue mentioned in the comment on clickOnContextMenuItem? Are the test servers used in github not macos so it still runs regularly then? (PS test:apiAndUnit works for me on windows, so I guess test still passes right now)
Adds the ability to maintain several connections in the VSCode plugin.