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

Send initialization data in the NewReport message #2317

Merged
117 changes: 96 additions & 21 deletions frontend/src/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import React from "react"
import { shallow, mount, ReactWrapper } from "enzyme"
import { ForwardMsg } from "autogen/proto"
import { ForwardMsg, NewReport } from "autogen/proto"
import { IMenuItem } from "hocs/withS4ACommunication/types"
import { MetricsManager } from "./lib/MetricsManager"
import { getMetricsManagerForTest } from "./lib/MetricsManagerTestUtils"
Expand All @@ -28,17 +28,10 @@ import MainMenu from "./components/core/MainMenu"

const getProps = (extend?: Partial<Props>): Props => ({
screenCast: {
currentState: "OFF",
toggleRecordAudio: jest.fn(),
startRecording: jest.fn(),
stopRecording: jest.fn(),
fileName: "",
recording: false,
recordAudio: false,
countdown: -1,
startAnimation: false,
showRecordedDialog: false,
showScreencastDialog: false,
showUnsupportedDialog: false,
Comment on lines -34 to -41
Copy link
Contributor Author

@tconkling tconkling Nov 4, 2020

Choose a reason for hiding this comment

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

Unrelated to this PR, but I was making changes to App.tsx - none of these props exist

},
s4aCommunication: {
connect: jest.fn(),
Expand Down Expand Up @@ -84,7 +77,8 @@ describe("App", () => {
})

afterEach(() => {
SessionInfo.singleton = undefined
const UnsafeSessionInfo = SessionInfo as any
UnsafeSessionInfo.singleton = undefined
})

it("renders without crashing", () => {
Expand All @@ -93,22 +87,24 @@ describe("App", () => {
expect(wrapper.html()).not.toBeNull()
})

it("should reload when streamlit server version changes", () => {
it("reloads when streamlit server version changes", () => {
const props = getProps()
const wrapper = shallow(<App {...props} />)

window.location.reload = jest.fn()

const fwMessage = new ForwardMsg()

fwMessage.initialize = {
environmentInfo: {
streamlitVersion: "svv",
fwMessage.newReport = {
initialize: {
environmentInfo: {
streamlitVersion: "svv",
},
sessionId: "sessionId",
userInfo: {},
config: {},
sessionState: {},
},
sessionId: "sessionId",
userInfo: {},
config: {},
sessionState: {},
}

// @ts-ignore
Expand All @@ -117,7 +113,7 @@ describe("App", () => {
expect(window.location.reload).toHaveBeenCalled()
})

it("should start screencast recording when the MainMenu is clicked", () => {
it("starts screencast recording when the MainMenu is clicked", () => {
const props = getProps()
const wrapper = shallow(<App {...props} />)

Expand All @@ -135,7 +131,7 @@ describe("App", () => {
)
})

it("should stop screencast when esc is pressed", () => {
it("stops screencast when esc is pressed", () => {
const props = getProps()
const wrapper = shallow(<App {...props} />)

Expand All @@ -145,7 +141,7 @@ describe("App", () => {
expect(props.screenCast.stopRecording).toBeCalled()
})

it("should show s4aMenuItems", () => {
it("shows s4aMenuItems", () => {
const props = getProps({
s4aCommunication: {
connect: jest.fn(),
Expand All @@ -167,3 +163,82 @@ describe("App", () => {
])
})
})

describe("App.handleNewReport", () => {
const NEW_REPORT = new NewReport({
initialize: {
userInfo: {
installationId: "installationId",
installationIdV1: "installationIdV1",
installationIdV2: "installationIdV2",
email: "email",
},
config: {
sharingEnabled: false,
gatherUsageStats: false,
maxCachedMessageAge: 0,
mapboxToken: "mapboxToken",
allowRunOnSave: false,
},
environmentInfo: {
streamlitVersion: "streamlitVersion",
pythonVersion: "pythonVersion",
},
sessionState: {
runOnSave: false,
reportIsRunning: false,
},
sessionId: "sessionId",
commandLine: "commandLine",
},
})

afterEach(() => {
const UnsafeSessionInfo = SessionInfo as any
UnsafeSessionInfo.singleton = undefined
})

it("performs one-time initialization", () => {
const wrapper = shallow(<App {...getProps()} />)
const app = wrapper.instance()

const oneTimeInitialization = jest.spyOn(
app,
// @ts-ignore
"handleOneTimeInitialization"
)

expect(SessionInfo.isSet()).toBe(false)

// @ts-ignore
app.handleNewReport(NEW_REPORT)

expect(oneTimeInitialization).toHaveBeenCalledTimes(1)
expect(SessionInfo.isSet()).toBe(true)
})

it("performs one-time initialization only once", () => {
const wrapper = shallow(<App {...getProps()} />)
const app = wrapper.instance()

const oneTimeInitialization = jest.spyOn(
app,
// @ts-ignore
"handleOneTimeInitialization"
)

expect(SessionInfo.isSet()).toBe(false)

// @ts-ignore
app.handleNewReport(NEW_REPORT)
// @ts-ignore
app.handleNewReport(NEW_REPORT)
// @ts-ignore
app.handleNewReport(NEW_REPORT)

// Multiple NEW_REPORT messages should not result in one-time
// initialization being performed more than once.
expect(oneTimeInitialization).toHaveBeenCalledTimes(1)
expect(SessionInfo.isSet()).toBe(true)
})
})
107 changes: 44 additions & 63 deletions frontend/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
SessionEvent,
WidgetStates,
SessionState,
Config,
} from "autogen/proto"

import { RERUN_PROMPT_MODAL_DIALOG } from "lib/baseconsts"
Expand Down Expand Up @@ -287,14 +288,12 @@ export class App extends PureComponent<Props, State> {

try {
dispatchProto(msgProto, "type", {
initialize: (initializeMsg: Initialize) =>
this.handleInitialize(initializeMsg),
newReport: (newReportMsg: NewReport) =>
this.handleNewReport(newReportMsg),
sessionStateChanged: (msg: SessionState) =>
this.handleSessionStateChanged(msg),
sessionEvent: (evtMsg: SessionEvent) =>
this.handleSessionEvent(evtMsg),
newReport: (newReportMsg: NewReport) =>
this.handleNewReport(newReportMsg),
delta: (deltaMsg: Delta) =>
this.handleDeltaMsg(
deltaMsg,
Expand Down Expand Up @@ -378,64 +377,6 @@ export class App extends PureComponent<Props, State> {
})
}

/**
* Handler for ForwardMsg.initialize messages
* @param initializeMsg an Initialize protobuf
*/
handleInitialize = (initializeMsg: Initialize): void => {
const {
Comment on lines -381 to -386
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic now lives in the "handleOneTimeInitialization" function

sessionId,
environmentInfo,
userInfo,
config,
sessionState,
} = initializeMsg

if (
sessionId == null ||
!environmentInfo ||
!userInfo ||
!config ||
!sessionState
) {
throw new Error("InitializeMsg is missing a required field")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this part isn't necessary now. Are we just assuming the correct message is sent down (totally fine, but just checking)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these checks happen within the SessionInfo constructor, so this was just essentially duplicate logic

}

if (App.hasStreamlitVersionChanged(initializeMsg)) {
window.location.reload()
return
}

SessionInfo.current = new SessionInfo({
sessionId,
streamlitVersion: environmentInfo.streamlitVersion,
pythonVersion: environmentInfo.pythonVersion,
installationId: userInfo.installationId,
installationIdV1: userInfo.installationIdV1,
installationIdV2: userInfo.installationIdV2,
authorEmail: userInfo.email,
maxCachedMessageAge: config.maxCachedMessageAge,
commandLine: initializeMsg.commandLine,
userMapboxToken: config.mapboxToken,
})

MetricsManager.current.initialize({
gatherUsageStats: Boolean(config.gatherUsageStats),
})

MetricsManager.current.enqueue("createReport", {
pythonVersion: SessionInfo.current.pythonVersion,
})

this.setState({
sharingEnabled: Boolean(config.sharingEnabled),
allowRunOnSave: Boolean(config.allowRunOnSave),
})

this.props.s4aCommunication.connect()
this.handleSessionStateChanged(sessionState)
}

/**
* Handler for ForwardMsg.sessionStateChanged messages
* @param stateChangeProto a SessionState protobuf
Expand Down Expand Up @@ -530,9 +471,24 @@ export class App extends PureComponent<Props, State> {
* @param newReportProto a NewReport protobuf
*/
handleNewReport = (newReportProto: NewReport): void => {
const initialize = newReportProto.initialize as Initialize
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, does the protobuf not return the correct type here?

Copy link
Contributor Author

@tconkling tconkling Nov 10, 2020

Choose a reason for hiding this comment

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

this is the larger protobufjs typescript issue: message fields are typed as interfaces, not classes (IInitialize instead of Initialize), and the interface typings claim that fields can be undefined - when in fact, for a deserialized protobuf, there's no such thing as an undefined field (because anything not explicitly defined gets assigned a default value). So the cast here prevents us from doing a whole lot of non-null assertions on the message fields.


if (App.hasStreamlitVersionChanged(initialize)) {
window.location.reload()
return
}

// First, handle initialization logic. Each NewReport message has
// initialization data. If this is the _first_ time we're receiving
// the NewReport message, we perform some one-time initialization.
if (!SessionInfo.isSet()) {
// We're not initialized. Perform one-time initialization.
this.handleOneTimeInitialization(initialize)
}

const { reportHash } = this.state
const {
id: reportId,
reportId,
name: reportName,
scriptPath,
deployParams,
Expand Down Expand Up @@ -561,6 +517,31 @@ export class App extends PureComponent<Props, State> {
}
}

/**
* Performs one-time initialization. This is called from `handleNewReport`.
*/
handleOneTimeInitialization = (initialize: Initialize): void => {
SessionInfo.current = SessionInfo.fromInitializeMessage(initialize)

const config = initialize.config as Config

MetricsManager.current.initialize({
gatherUsageStats: config.gatherUsageStats,
})

MetricsManager.current.enqueue("createReport", {
pythonVersion: SessionInfo.current.pythonVersion,
})

this.setState({
sharingEnabled: config.sharingEnabled,
allowRunOnSave: config.allowRunOnSave,
})

this.props.s4aCommunication.connect()
this.handleSessionStateChanged(initialize.sessionState)
}

/**
* Handler for ForwardMsg.reportFinished messages
* @param status the ReportFinishedStatus that the report finished with
Expand Down
40 changes: 40 additions & 0 deletions frontend/src/lib/SessionInfo.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,47 @@
*/

import { SessionInfo } from "lib/SessionInfo"
import { Initialize } from "../autogen/proto"
tconkling marked this conversation as resolved.
Show resolved Hide resolved

test("Throws an error when used before initialization", () => {
expect(() => SessionInfo.current).toThrow()
})

test("Can be initialized from a protobuf", () => {
const MESSAGE = new Initialize({
userInfo: {
installationId: "installationId",
installationIdV1: "installationIdV1",
installationIdV2: "installationIdV2",
email: "email",
},
config: {
sharingEnabled: false,
gatherUsageStats: false,
maxCachedMessageAge: 31,
mapboxToken: "mapboxToken",
allowRunOnSave: false,
},
environmentInfo: {
streamlitVersion: "streamlitVersion",
pythonVersion: "pythonVersion",
},
sessionState: {
runOnSave: false,
reportIsRunning: false,
},
sessionId: "sessionId",
commandLine: "commandLine",
})

const si = SessionInfo.fromInitializeMessage(MESSAGE)
expect(si.sessionId).toEqual("sessionId")
expect(si.streamlitVersion).toEqual("streamlitVersion")
expect(si.pythonVersion).toEqual("pythonVersion")
expect(si.installationId).toEqual("installationId")
expect(si.installationIdV1).toEqual("installationIdV1")
expect(si.installationIdV2).toEqual("installationIdV2")
expect(si.authorEmail).toEqual("email")
expect(si.maxCachedMessageAge).toEqual(31)
expect(si.commandLine).toEqual("commandLine")
})