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

add default instruments selected run number #203

Merged
merged 5 commits into from
Feb 18, 2019
Merged

add default instruments selected run number #203

merged 5 commits into from
Feb 18, 2019

Conversation

Archerlly
Copy link
Contributor

@Archerlly Archerlly commented Feb 1, 2019

this's will lack com.apple.xray.owner.template in instruments archive data where run instruments with command line.
like:

  1. runinstruments -t Template.tracetemplate -D demo.trace -l 10000 -w test.app
  2. drag demo.trace into https://www.speedscope.app
  3. alert Unrecognized format! See documentation about supported formats

@coveralls
Copy link

coveralls commented Feb 1, 2019

Coverage Status

Coverage increased (+0.2%) to 46.817% when pulling f73fed1 on Archerlly:master into ddc6130 on jlfwong:master.

Copy link
Owner

@jlfwong jlfwong left a comment

Choose a reason for hiding this comment

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

Hi @Archerlly,

Thanks for the submission!

Do you have a small .trace file that you can include as a test to ensure that this behavior doesn't regress in the future?

I should be able to create this myself, though I'm not sure where to find a .tracetemplate file to create this.

@@ -361,7 +361,11 @@ async function readFormTemplate(tree: TraceDirectoryTree): Promise<FormTemplateD
const archive = readInstrumentsKeyedArchive(await readAsArrayBuffer(formTemplate))

const version = archive['com.apple.xray.owner.template.version']
const selectedRunNumber = archive['com.apple.xray.owner.template'].get('_selectedRunNumber')
let selectedRunNumber = 1
if ('com.apple.xray.owner.template' in Object.keys(archive)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is quite right. The in test is for objects, not arrays, and Object.keys returns an array, so I think this condition will always be false.

image

Instead, I think you want:

Suggested change
if ('com.apple.xray.owner.template' in Object.keys(archive)) {
if ('com.apple.xray.owner.template' in archive) {

let selectedRunNumber = 1
if ('com.apple.xray.owner.template' in Object.keys(archive)) {
selectedRunNumber = archive['com.apple.xray.owner.template'].get('_selectedRunNumber')
console.log('get selectedRunNumber', selectedRunNumber)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm guessing this was useful for debugging, but can you please remove this before we merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • .tracetemplate file create by Instruments File -> Save As Template
  • console.log('get selectedRunNumber', selectedRunNumber) is debug code and have removed
  • add a testing with small .trace file that without selected run number

Thanks for your suggestion

@Archerlly Archerlly closed this Feb 15, 2019
@Archerlly Archerlly reopened this Feb 15, 2019
@Archerlly
Copy link
Contributor Author

  • remove * console.log('get selectedRunNumber', selectedRunNumber)
  • remove Object.keys for in func
  • add a testing with small .trace file that without selected run number

Thanks for your suggestion

@jlfwong jlfwong merged commit abd74be into jlfwong:master Feb 18, 2019
@jlfwong
Copy link
Owner

jlfwong commented Feb 18, 2019

@Archerlly Your fix has now been published to npm and deployed to https://speedscope.app/. Thanks for the fix!

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.

3 participants