-
Notifications
You must be signed in to change notification settings - Fork 6
Fix mocking #15
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
base: master
Are you sure you want to change the base?
Fix mocking #15
Conversation
src/main/java/com/teragrep/cfe_16/bo/TimestampedHttpEventData.java
Outdated
Show resolved
Hide resolved
src/test/java/com/teragrep/cfe_16/bo/TimestampedHttpEventDataTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/teragrep/cfe_16/bo/TimestampedHttpEventDataTest.java
Outdated
Show resolved
Hide resolved
please rebase |
Rebase done |
pom.xml
Outdated
<dependency> | ||
<groupId>systems.manifold</groupId> | ||
<artifactId>manifold-all</artifactId> | ||
<groupId>systems.manifold</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is manifold needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not, project compiles and verifies without the plugin.
Should a new issue be made for the removal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please
@@ -1,17 +1,47 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license must not change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License reverted in c7b9490
src/main/resources/log4j2.xml
Outdated
@@ -1,20 +1,66 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<!-- | |||
~ HTTP Event Capture to RFC5424 CFE_16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove dashes at line start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 11148e0
else { | ||
final String eventString = event.toString(); | ||
DefaultHttpEventData defaultHttpEventData = new DefaultHttpEventData(); | ||
defaultHttpEventData.setEvent(eventString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why setter instead of ctor arg?
String jsonObjectStr = parser.next().toString(); | ||
eventData = verifyJsonData(jsonObjectStr, previousEvent); | ||
eventData = assignMetaData(eventData, authToken, channel); | ||
assignMetaData(eventData, authToken, channel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method is using setters to modify method call args, which is horrible. please remove the method and move at least the calling code here.
|
||
/** | ||
* Converts the given time stamp into epoch milliseconds. If the time value | ||
* in the object has 13 digits, it means that time has been already given in epoch milliseconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why guesswork is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked the original writer about the origin of this, but there wasn't a conclusive answer.
Should we trust that the incoming timestamp is in correct format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check first if numeric only, if numeric only, then it is epoch, otherwise it might be known timestamp, try conversion to the known format and if it fails generate one for the time of the receive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see above comments and reply. I will check tests after the /main/ is resolved
- Cleans up the EventTime.asTime() if-else tree
- Old Time classes have been deleted - Tests are missing and broken at the moment
…oSyslogMessageTest
…alue - Allows for tests to declare an expected value - Tested by running the test file 1000 times
- Causes problems if the test is not run in 30 seconds
- Some vars were not renamed when refactoring objects, so fix the HttpEvent... references
- Uncomplicates the sendEvents method
Fixes #27