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

Convert the notification to use notiffany gem #106

Merged
merged 10 commits into from Oct 10, 2015

Conversation

acant
Copy link
Collaborator

@acant acant commented Sep 11, 2015

Also going to do some refactoring and clean up in the process. Specifically I hope that I will make the integration tests faster and more reliable.

@nesquena
Copy link
Owner

What happened with the tests? https://travis-ci.org/nesquena/gitdocs/builds/79777537 are they just broken in general?

@acant
Copy link
Collaborator Author

acant commented Sep 12, 2015

Frustratingly, no.
The tests are passing locally but failing on TravisCI. Seemingly on timing related issues. I have been playing with the timeouts and trying to simplify the test code, which hopefully will either fix the failures or reveal the problems.

If you have time, could you pull this branch and run the tests? It would give me another data point, and might help clarify if this is really specific to the TravisCI environment.

@acant
Copy link
Collaborator Author

acant commented Sep 19, 2015

I am uploading some temporary commits to try and gather more information about the failures on TravisCI. They will be removed before merging.
The commits are intended to display more information logging information than usual.

@nesquena
Copy link
Owner

Ok thanks for the heads up, sorry travis is giving you so much trouble.

@acant
Copy link
Collaborator Author

acant commented Sep 19, 2015

np. Hopefully by the time I'm finish, I'll have a better understanding of...something. :)
My worst case scenario is to just disable the failing tests in Travis, but there is already one case of that. It would be a shame to add more.

* add DRY gitdocs execution into #gitdocs_command and name the wrapper
  methods consistently
* remove some unnecessary calls to abs_current_dir when asserting
  output
* make the pid_file and gitdocs binary paths absolute so they are not
  affect by the setting of the current directory
* consolidate the #before_setup and #after_teardown methods
* replace #git_clone_and_gitdocs_add with #gitdocs_create, which
  eliminates some extra code and more more through the gitdocs CLI
  which makes the integration test more consistent
DRY the duplicate from the units tests for fabricating and making
assertions about git repositories.
Add GitFactory.append in the process.
Remove methods from the integration/test_helper which are no longer
used.
I think that will be useful, particularly for finding errors in Travis.
On a test failure the log file and the current aruba directory will be
printed.
The verbose flag will increase the log level to DEBUG, which will also
additional debugging logs to be added to the code, but hidden during
normal operations.
Now that the --verbose flag exists to increase log level, I think that
this is a more accurate name for this flag.
Add logging methods to Gitdocs to reduce method chains, and update the
existing log calls to best specify appropriate log levels.
...the notifications will be logged even if the notifications
themselves are not being displayed.
Because the HOME directory is reset for these tests git would not use
an existing global configuration. Write the user.name and user.email
into the testing home directory, so valid value will be present when
committing during the integration tests.
@acant
Copy link
Collaborator Author

acant commented Oct 10, 2015

And the test are passing! The problem was that git was not finding a user.email and user.name setting when running the integration tests in TravisCI. My wonder if the TravisCI image used to have those values set in the system config, and now it does not.
Explicitly, setting them in the test HOME directory will be generally more reliable.

@nesquena I have re-based the existing commits so they are ready to merge if you like. And I'll continue with the notification changes here or in another PR.

@nesquena
Copy link
Owner

Sweet! Great news.

nesquena added a commit that referenced this pull request Oct 10, 2015
Convert the notification to use notiffany gem
@nesquena nesquena merged commit 6d0dddd into nesquena:master Oct 10, 2015
@acant
Copy link
Collaborator Author

acant commented Oct 11, 2015

Thanks @nesquena. I just updated the README and CHANGELOG in master, so everything should be up to date now.

@nesquena
Copy link
Owner

Nice!

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.

None yet

2 participants