Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Micropilot #73

Merged
merged 30 commits into from
Apr 17, 2013
Merged

Micropilot #73

merged 30 commits into from
Apr 17, 2013

Conversation

monicachew
Copy link

This is just some placeholder code, but I could use some testing advice. Also please review the changes to the README to see if setup directions make sense.

@ghost ghost assigned mozkeeler Apr 11, 2013
@monicachew
Copy link
Author

For testing, I'd like to somehow trigger the upload and check that the events emitted by the tests are what we expect. I guess I could expose forceUpload for testing purposes, then yield to the promise returned by monitor.upload. Is there a way to expose forceUpload only to the test?

],
"volo": {
"dependencies": {
"packages/micropilot": "github:gregglind/micropilot/v0.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting this to automatically fetch micropilot for me. Too much to ask, or do I not have volo set up correctly?

Copy link
Author

Choose a reason for hiding this comment

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

I think, but am not sure, that this means that if we use volo to install blushproof itself then volo will fetch packages/micropilot, e.g.

volo create myaddon micropilot-template

However, I'm not sure how/if we can take advantage of that in the development cycle.

@gregglind, any clues?

Copy link
Author

Choose a reason for hiding this comment

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

This doesn't seem to be supported: volojs/volo#149

Seems like the intended workflow is to manually call volo add -f when you want to update the dependency.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, figure out workflow by reading bugs: volojs/volo#22

@jrburke, is the expected workflow to contribute to a git-managed codebase with volo dependencies to

git clone myproject
cd myproject
volo add

?

@mozkeeler
Copy link
Contributor

I don't know of a way to expose something only to the tests. There are already some testing-only custom events that are emitted that in theory regular code could listen for, so I don't think it's that bad to expose a testing-only thing to non-tests. Maybe we should add annotations? Also, it would be cool to have a way of replacing the micropilot server with localhost:4444 and registering a handler that will check that the data it receives is good (the latter part should be pretty easy - see https://developer.mozilla.org/en-US/docs/Httpd.js/HTTP_server_for_unit_tests ).

@monicachew
Copy link
Author

So, if simulate = true in the upload then we get the data back in the response, which I think is good enough for testing we are recording what we expect. I think we should differentiate between

  1. Testing that micropilot upload works correctly (https://github.com/gregglind/micropilot/blob/master/test/test-micropilot-upload.js)
  2. Testing that we record what we expect

Otherwise unit tests devolve into integration tests.

@monicachew
Copy link
Author

I ended up doing a massive rewrite of the test code to use promises instead of continuation callbacks. It is shorter and reads more synchronously, so I think it's easier to understand -- but I am sorry it ended up being so large.

This is more or less ready for review.

deferred.resolve(true);
return deferred.promise;
}).
// This clear is not working.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something I have a question about that we should discuss during the meeting today.

@monicachew
Copy link
Author

Fixed everything (I think) besides the prototype issue, which I still haven't figured out. OK to merge?

mozkeeler added a commit that referenced this pull request Apr 17, 2013
@mozkeeler mozkeeler merged commit 3dfc5d3 into master Apr 17, 2013
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants