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.

Sign up for GitHub

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

Jump to bottom

Update and fix test suite #174

Merged
merged 4 commits into from
Jan 30, 2020
Merged

Update and fix test suite #174

merged 4 commits into from
Jan 30, 2020

Conversation

clhunsen
Copy link
Collaborator

@clhunsen clhunsen commented Jan 24, 2020 •
edited
Loading

Prerequisites

  • I adhere to the coding conventions (described here) in my code.
  • I have updated the copyright headers of the files I have modified.
  • I have written appropriate commit messages, i.e., I have recorded the goal, the need, the needed changes, and the location of my code modifications for each commit. This includes also, e.g., referencing to relevant issues.
  • I have put signed-off tags in all commits.
  • I have updated the changelog file NEWS.md appropriately.
  • The pull request is opened against the branch dev.

Description

This PR is mainly concerned with the test suite and compatibility with Travis CI:

  • fix tests failing for R 3.3,
  • add R 3.6 to test suite, and
  • improve .travis.yml to fix warnings.

Changelog

Changed/Improved

Fixed

  • Ensure sorting of commit-count and LOC-count data.frames to fix tests with R 3.3 (33d63fd)

@clhunsen
Add R version 3.6 to test suite
8b2a52d 
This is related to issue se-sic#161.

Signed-off-by: Claus Hunsen <[email protected]>
@clhunsen clhunsen added this to the v3.6 milestone Jan 24, 2020
@clhunsen
Ensure sorting of commit-count and LOC-count data.frames
33d63fd 
Due to weird circumstances, tests are failing only on R 3.3, indicating
string mismatches in all commit-count and LOC-count data.frames [1]. The
reason for these failures is that the row ordering of the respective
data.frames is not deterministic for authors exhibiting the same
frequency value.

To fix this problem, the resulting data.frames of all corresponding
functions are ordered by the author and committer names, respectively.

Props to @hechtlC for a valuable discussion.

[1] https://travis-ci.com/se-passau/coronet/jobs/278309743

Signed-off-by: Claus Hunsen <[email protected]>
@clhunsen
Update .travis.yml
41ce589 
The configuration file for Travis CI caused some warnings in the Travis
backend (see also https://config.travis-ci.com/explore):
> [warn] on root: deprecated key: "sudo" (The key `sudo` has no effect
> anymore.)
> [info] on root: missing os, using the default "linux"

To fix these warnings, the configuration is heavily adapted:
- Use Linux OS explicitly.
- Use distribution 'xenial' (16.04) instead of 'trusty'.
- Re-order and comment configuration parts.

Signed-off-by: Claus Hunsen <[email protected]>
@clhunsen
Update changelog
4220a79 
Signed-off-by: Claus Hunsen <[email protected]>
@clhunsen clhunsen marked this pull request as ready for review January 24, 2020 20:33
@clhunsen clhunsen requested review from bockthom and hechtlC January 24, 2020 20:33
hechtlC
hechtlC approved these changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@hechtlC hechtlC left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fix and the enhancement regarding the R versions @clhunsen .
The changes look good to me and I don't see any issues. So I would be fine with merging.

@bockthom
Copy link
Collaborator

Nice work @clhunsen. Thanks a lot!

I am a little bit surprised that we did not add the commit regarding R version 3.6 earlier - the commit is half a your old. But better late than never ;)

Regarding the failing tests on R 3.3: Why didn't we recognize this earlier? The tests seem to have failed on the code of the last pull request. That's weird and should not happen again. @clhunsen Do you have any suggestions on how to recognize this earlier? How did you recognize that? May be helpful to get some advice from you for future development.

This pull request looks good. So I will merge it this week, right after you will have replied to my questions ;)

@clhunsen
Copy link
Collaborator Author

I am a little bit surprised that we did not add the commit regarding R version 3.6 earlier - the commit is half a y[ea]r old. But better late than never ;)

I just forgot about that commit and just "re-found" while rebasing my development branch last week. 😉😅

Regarding the failing tests on R 3.3: Why didn't we recognize this earlier? The tests seem to have failed on the code of the last pull request. That's weird and should not happen again. @clhunsen Do you have any suggestions on how to recognize this earlier? How did you recognize that? May be helpful to get some advice from you for future development.

This is (potentially) caused by an update of an R package that is a dependency of the package sqldf. With that package update, the tests started to fail: not only for the upstream development branch but also my dev branch on my fork (although, mysteriously only causing issues with R 3.3).

I realized the failures because the cron jobs of Travis CI failed (see here and here, look for the label cron). After that @hechtlC and I debugged and discussed a little, coming up with the changes of this PR then.
Maybe, @ChristianKaltenecker can grant you access to the Travis app for this repository. (I am not sure anymore how this exactly works...)

This pull request looks good. So I will merge it this week, right after you will have replied to my questions ;)

Then, this PR is fine for merging now. 😁

@clhunsen clhunsen mentioned this pull request Jan 30, 2020
Closed
6 tasks
@bockthom
Copy link
Collaborator

I realized the failures because the cron jobs of Travis CI failed (see here and here, look for the label cron). After that @hechtlC and I debugged and discussed a little, coming up with the changes of this PR then.
Maybe, @ChristianKaltenecker can grant you access to the Travis app for this repository. (I am not sure anymore how this exactly works...)

I already have access to the Travis CI jobs. As I also can reach the settings page an travis, I think that I already have full access on that (at least, it looks like that, don't know if there is more than what I can see).

Regarding how you realized the failures: Did you access the travis page for cron jobs on purpose or by chance? Or do you get regular notifications for them?
I can see all the cron job runs on travis, but I never had a look at them before. So, it may be a good idea to regularly check for failing jobs ;)

@clhunsen
Copy link
Collaborator Author

Regarding how you realized the failures: Did you access the travis page for cron jobs on purpose or by chance? Or do you get regular notifications for them?

I enabled email notifications here. Thus, I get emails when jobs are failing. The failing jobs bugged me for quite some time already, I just had no time to fix the underlying issue until now.

I can see all the cron job runs on travis, but I never had a look at them before. So, it may be a good idea to regularly check for failing jobs ;)

That may be a good idea. 😀👍 You can also add https://travis-ci.com/dashboard to your bookmarks and regularly check for obscurities. 😉 (The link is especially useful if you have several repositories on Travis.)

bockthom
bockthom approved these changes Jan 30, 2020
View reviewed changes
Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. Thanks for your effort, your fixes, and your explanations to me @clhunsen.

@bockthom bockthom merged commit ba06271 into se-sic:dev Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bockthom bockthom bockthom approved these changes

@hechtlC hechtlC hechtlC approved these changes

Assignees
No one assigned
Labels
bug enhancement
Projects
None yet
Milestone
v3.6
Development

Successfully merging this pull request may close these issues.
3 participants
@clhunsen @bockthom @hechtlC