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

refactor: use public accessors for network records #1588

Closed
wants to merge 1 commit into from

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Jan 31, 2017

Brought over from discussion

The two biggest holdouts are _resourceType and _initiator which are functions and not getter properties.

@brendankenny
Copy link
Member

nice, though this does move the problem mentioned in #1513 (comment) to the tests, so if the implementation of the getters change the test fixtures will presumably have to be updated.

Not sure how much of a danger that is, though, and I'd say simulating the getters in the test fixtures is definitely better than accessing the internal properties in the actual gatherers/audits...

@brendankenny
Copy link
Member

after discussion today, sounds like the solution is this PR + storing performance logs (or whatever format) for network activity for tests. Tests would use something like NetworkRecorder.recordsFromLogs() to convert them into the networkRecords format gatherers and audits expect, and we can drop all use of the internal variable names for tests.

Main things we'd need to establish would be an easy format to create for creating test files and (if we don't end up using the performance logs we currently support) an easy function for conversion to networkRecords

@paulirish
Copy link
Member

honestly we're not in a big rush here. :)
let's drop this for now.

@paulirish paulirish closed this Mar 8, 2017
@paulirish paulirish deleted the network_public_accessors branch May 4, 2017 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants